-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
fips: explicitly setup cpuid when initialize #24419
Conversation
Fixes: openssl#23979 Previously fips module relies on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via __attribute__((constructor)). This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages: 1. Not all platform/toolchain supports such behavior 2. Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called 3. Implicit path is hard to maintain and debug
This means OPENSSL_cpuid_setup() will be called twice on platforms that have it as a constructor/DEP init. Should we drop the attribute from it and call it from another call marked as constructor, that won't be compiled into FIPS_MODULE? |
IMO that should be done at least for the master branch. For stable branches we should IMO skip this call on platforms which mark it as constructor to minimize the changes. |
I think libcrypto also does not need such another constructor as Also, calling cpuid_setup twice should not be harmful. |
The problem is that this init function might not be called in some circumstances such as when only the deprecated low-level functions are being called by an application.
Yeah, that's right. And given it is already called twice for libcrypto, let's ignore this issue for now. |
This pull request is ready to merge |
Merged to all the active branches. Thank you for your contribution. |
Is this fix already pushed ? When will it be available in branch ? |
Fixes: #23979 Previously fips module relied on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via __attribute__((constructor)). This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages: 1. Not all platform/toolchain supports such behavior 2. Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called 3. Implicit path is hard to maintain and debug Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24419) (cherry picked from commit a192b24)
Fixes: #23979 Previously fips module relied on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via __attribute__((constructor)). This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages: 1. Not all platform/toolchain supports such behavior 2. Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called 3. Implicit path is hard to maintain and debug Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24419) (cherry picked from commit a192b24)
Fixes: #23979 Previously fips module relied on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via __attribute__((constructor)). This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages: 1. Not all platform/toolchain supports such behavior 2. Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called 3. Implicit path is hard to maintain and debug Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24419) (cherry picked from commit a192b24)
Fixes: #23979 Previously fips module relied on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via __attribute__((constructor)). This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages: 1. Not all platform/toolchain supports such behavior 2. Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called 3. Implicit path is hard to maintain and debug Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24419)
Fixes: #23979 Previously fips module relied on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via __attribute__((constructor)). This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages: 1. Not all platform/toolchain supports such behavior 2. Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called 3. Implicit path is hard to maintain and debug Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24419) (cherry picked from commit a192b24)
Fixes: #23979
Previously fips module relies on OPENSSL_cpuid_setup being used as constructor by the linker to correctly setup the capability vector, either via .section .init (for x86_64) or via
__attribute__((constructor))
.This would make ld.so call OPENSSL_cpuid_setup before the init function for fips module. However, this early constructing behavior has several disadvantages:
Not all platform/toolchain supports such behavior
Initialisation sequence is not well defined, and some function might not be initialized when cpuid_setup is called
Implicit path is hard to maintain and debug
Test
I have tested this branch merged with #24403, and the riscvcap_P inside libcrypto/fips.so are both initialized.
Uncertainties
openssl/crypto/init.c
Lines 68 to 74 in a6afe2b
This behavior should be documented somewhere, where should I put them.
Test and coverage for this also needs massive effort. The coverage is also a problem when we gradually remove the constructor behavior of all cpuid_setup (Set OPENSSL_ppccap_P global variable in fips provider context #24399 (comment))
Checklist