Add fix for memset_s issue discovered with ATFE#776
Conversation
Signed-off-by: Ben Taylor <ben.taylor@linaro.org>
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
We need to fix an interoperability problem, not remove a feature.
| #define TF_PSA_CRYPTO_TF_PSA_CRYPTO_PLATFORM_REQUIREMENTS_H | ||
|
|
||
| /* Do not try to use the library extension with ATfE toolchain */ | ||
| #if !defined(__GNUC__) || !defined(__clang_major__) |
There was a problem hiding this comment.
This is throwing the baby with the bathwater. We do want to enable __STDC_WANT_LIB_EXT1__ so that we can use Annex K functions (memset_s, gmtime_s, …) where available. That inclues Clang with Glibc.
A conforming C implementation will not define __STDC_LIB_EXT1__ in its headers if it doesn't provide the Annex K functions. This should be the case even for embedded platforms that don't have a full libc. Technically the fact that compilation fails is a bug in TF-M or in AtFE, but given that TF-M is not using an external libc, we may need to add a special case.
We need to identify why AtFE in TF-M has a problem, and either add a workaround for this specific case or tell TF-M how to fix their code or their compiler invocation.
There was a problem hiding this comment.
Apologies, I thought the plan was to verify the error and apply their fix.
So the issue is caused because TF-M adds the option to the compiler -nostdlib here https://github.com/TrustedFirmware-M/trusted-firmware-m/blob/c630c687aecaba792788b3d0cec17035981e0eb9/toolchain_ATFE.cmake#L134. If we do not want to accept their patch, they will have to change their build to something like this pattern:
clang --target=arm-none-eabi -mcpu=cortex-m33 -mthumb -nostartfiles \ tf-psa-crypto_build/repro_zeroize.c \ tf-psa-crypto_build/libc_stubs.c \ f-psa-crypto_build/core/libtfpsacrypto.a \ -Wl,-e,main \ -Wl,--defsym=__heap_start=0x20000000 \ -Wl,--defsym=__heap_end=0x20000000 \ -o tf-psa-crypto_build/repro_zeroize_option1.axf
Though they may not be keen to do that, as I suspect this will increase the size of their binaries and they won't like that. Which is probably why they're using the option in the first place. We will also probably have to add a stub something like this:
void _exit(int status) { (void) status; for (;;) { } }
Let me know what you think?
There was a problem hiding this comment.
Why is there a problem only with AtFE, not with other toolchains?
There was a problem hiding this comment.
So, we may have to work with tf-m to figure this out for sure as it appears to be a larger architectural problem with tf-m and only they will potentially know the exact reasoning behind their build config. However -nostdlib is gated on CONFIG_TFM_INCLUDE_STDLIBC , this is forced to be off when the compiler is clang here https://github.com/TrustedFirmware-M/trusted-firmware-m/blob/c630c687aecaba792788b3d0cec17035981e0eb9/config/check_config.cmake#L22 It appears based on this commit https://git.trustedfirmware.org/plugins/gitiles/TF-M/trusted-firmware-m/+/4537a2c584e1a496a215f37eecc22d37b1d9c22f that they are attempting to replace the stdlib with their own smaller more controlled implementation. This is being sourced from different places for different toolchains, which adds another level of variables and further explains the different behavior. However it looks like they are trying to add their own glibc stubs here https://github.com/TrustedFirmware-M/trusted-firmware-m/blob/c630c687aecaba792788b3d0cec17035981e0eb9/bl2/CMakeLists.txt#L162 and here https://github.com/TrustedFirmware-M/trusted-firmware-m/blob/main/bl2/src/mbedcrypto_stubs.c. However they have not added implementations for the functions that are currently causing the issue. So there's a few different ways they could fix it, though the exact solution will depend on their architectural objectives.
Description
Add fix for memset_s issue discovered with ATFE resolves #754. Adds a patch currently being used by TF-M. Open issues are do we want to add additional testing to cover their use cases and do we want to backport it? Let me know what people think?
PR checklist