Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set OPENSSL_ppccap_P global variable in fips provider context #24399

Closed
wants to merge 0 commits into from

Conversation

sanumesh
Copy link
Contributor

@sanumesh sanumesh commented May 14, 2024

Call OPENSSL_cpuid_setup while loading fips provider so that the value of OPENSSL_ppccap_P global variable gets set with hardware capabilities

This is to address issue : #23979

CLA: trivial

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 14, 2024
@sanumesh sanumesh changed the title Update 10-main.conf Set OPENSSL_ppccap_P global variable in fips provider context May 14, 2024
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does bypassing the constructor (DEP) _init in providers/fips/self_test.c do?

@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 labels May 15, 2024
@t8m
Copy link
Member

t8m commented May 15, 2024

This would be acceptable with CLA: trivial. Could you please amend the commit message body to add CLA: trivial on a separate line?

ZenithalHourlyRate pushed a commit to ZenithalHourlyRate/openssl that referenced this pull request May 15, 2024
Call OPENSSL_cpuid_setup while loading fips provider so that the value of OPENSSL_ppccap_P global varaible gets set with hardware capabilities

CLA: trivial

Borrowed from openssl#24399 to test whether it is the issue
ZenithalHourlyRate pushed a commit to ZenithalHourlyRate/openssl that referenced this pull request May 15, 2024
Call OPENSSL_cpuid_setup while loading fips provider so that the value of OPENSSL_ppccap_P global varaible gets set with hardware capabilities

CLA: trivial

Borrowed from openssl#24399 to test whether it is the issue
@paulidale
Copy link
Contributor

What does bypassing the constructor (DEP) _init in providers/fips/self_test.c do?

I don't think this bypasses the DEP. It adds an additional constructor.
I suspect this would better be done by adding the initialisation call to the DEP.

@ZenithalHourlyRate
Copy link
Contributor

I suspect this would better be done by adding the initialisation call to the DEP.

Agreed. Check #24403 (comment) and #23978 (comment). My opinion is to remove all attribute constructor and use explicit initialisation. Now the constructor behavior differs among platforms (!_WIN32), toolchains (GNUC) and archs (riscvcap.c), and this PR adds another complexity by LDFLAGS -Wl, which is really hard to maintain.

Related snippets

openssl/crypto/init.c

Lines 55 to 69 in a6afe2b

DEFINE_RUN_ONCE_STATIC(ossl_init_base)
{
/* no need to init trace */
OSSL_TRACE(INIT, "ossl_init_base: setting up stop handlers\n");
#ifndef OPENSSL_NO_CRYPTO_MDEBUG
ossl_malloc_setup_failures();
#endif
if ((optsdone_lock = CRYPTO_THREAD_lock_new()) == NULL
|| (init_lock = CRYPTO_THREAD_lock_new()) == NULL)
goto err;
OPENSSL_cpuid_setup();

openssl/crypto/riscvcap.c

Lines 116 to 122 in a6afe2b

# if defined(__GNUC__) && __GNUC__>=2
__attribute__ ((constructor))
# endif
void OPENSSL_cpuid_setup(void)
{
char *e;

openssl/crypto/s390xcap.c

Lines 109 to 113 in a6afe2b

#if defined(__GNUC__) && defined(__linux)
__attribute__ ((visibility("hidden")))
#endif
void OPENSSL_cpuid_setup(void)
{

openssl/crypto/armcap.c

Lines 32 to 44 in a6afe2b

#ifdef _WIN32
void OPENSSL_cpuid_setup(void)
{
OPENSSL_armcap_P |= ARMV7_NEON;
OPENSSL_armv8_rsa_neonized = 1;
if (IsProcessorFeaturePresent(PF_ARM_V8_CRYPTO_INSTRUCTIONS_AVAILABLE)) {
// These are all covered by one call in Windows
OPENSSL_armcap_P |= ARMV8_AES;
OPENSSL_armcap_P |= ARMV8_PMULL;
OPENSSL_armcap_P |= ARMV8_SHA1;
OPENSSL_armcap_P |= ARMV8_SHA256;
}
}

openssl/crypto/armcap.c

Lines 59 to 68 in a6afe2b

#else /* !_WIN32 && __ARM_MAX_ARCH__ >= 7 */
/* 3 ways of handling things here: __APPLE__, getauxval() or SIGILL detect */
/* First determine if getauxval() is available (OSSL_IMPLEMENT_GETAUXVAL) */
# if defined(__GNUC__) && __GNUC__>=2
void OPENSSL_cpuid_setup(void) __attribute__ ((constructor));
# endif

@ZenithalHourlyRate
Copy link
Contributor

Another implicit path: #23978 (reply in thread)

@sanumesh
Copy link
Contributor Author

sanumesh commented May 16, 2024

What does bypassing the constructor (DEP) _init in providers/fips/self_test.c do?

I don't think this bypasses the DEP. It adds an additional constructor. I suspect this would better be done by adding the initialisation call to the DEP.

By adding this additional -binitfini in configuration file, we can ensure there is no modification done to fips provider code.
If we have to modify the initialization code in DEP _init , we may break fips certification leading to lab re-testing

@t8m
Copy link
Member

t8m commented May 16, 2024

This is really a mess. IMO we should clean it up properly (at least on the master branch).

  1. The OPENSSL_cpuid_setup() should not be a constructor by itself.
  2. In FIPS provider it should be explicitly called from OSSL_provider_init_int() before self tests are executed.
  3. In libcrypto, there should be a common constructor (OS specific, but not arch-specific) that executes OPENSSL_cpuid_setup().

@sanumesh sanumesh closed this May 16, 2024
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label May 16, 2024
@slontis
Copy link
Member

slontis commented May 17, 2024

If we have to modify the initialization code in DEP _init , we may break fips certification leading to lab re-testing

Unfortunately changing the config more than likely also breaks FIPS compliance. You cant just build however you want and still assume you are compliant (This is a deviation of the Security Policy instructions on how to build)
Also note that AIX is not a platform that was validated by OpenSSL.

@paulidale
Copy link
Contributor

2. In FIPS provider it should be explicitly called from OSSL_provider_init_int() before self tests are executed.
3. In libcrypto, there should be a common constructor (OS specific, but not arch-specific) that executes OPENSSL_cpuid_setup().

These two are the way to go.

Using a constructor dodges the FIPS change issue but it will cause other problems. I don't think we should do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants