Add fix for memset_s issue discovered with ATFE #776
Open
bjwtaylor wants to merge 1 commit from
bjwtaylor/tf-m-build-issues into development
pull from: bjwtaylor/tf-m-build-issues
merge into: gh-Mbed-TLS:development
gh-Mbed-TLS:development
gh-Mbed-TLS:tf-psa-crypto-1.1
gh-Mbed-TLS:archive/20261005T153337Z-force/pull/927
gh-Mbed-TLS:archive/20261001T141055Z-force/pull/927
gh-Mbed-TLS:archive/20261005T153337Z-force/pull/931
gh-Mbed-TLS:archive/20261001T083616Z-force/pull/927
gh-Mbed-TLS:archive/20260930T232327Z-force/pull/927
gh-Mbed-TLS:archive/20260930T154618Z-force/pull/927
gh-Mbed-TLS:archive/20260929T231919Z-rescue/rohankl-arm/mldsa-pubkey-899
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9342-public-key-import-export
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ManoharPaturi/fix-pem-ct-padding
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ManoharPaturi/fix-asn1write-null-guard
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ManoharPaturi/fix-pbes2-params-validation
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/blake2
gh-Mbed-TLS:archive/20260929T231919Z-rescue/jfdev001/v1.1.1_bump-cmake-minimum-version
gh-Mbed-TLS:archive/20260929T231919Z-rescue/jfdev001/bump-cmake-minimum-version
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ronald-cron-arm/sm3-base
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/blake2-hmac
gh-Mbed-TLS:archive/20260929T231919Z-rescue/v1rtu3x/enforce-hash-length-sign-verify
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/file-backed-keystore-doc
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9344-set-context
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9343-key-pair-import-export-depends-on-9342
gh-Mbed-TLS:archive/20260929T231919Z-rescue/romart/development
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sweetlilmre/constant-time-embedded-perf
gh-Mbed-TLS:archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-rsa-pss
gh-Mbed-TLS:archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-ecdsa
gh-Mbed-TLS:archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-tests
gh-Mbed-TLS:archive/20260929T231919Z-rescue/oubaid-a/dev_psa_driver_wrapper
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/keystoreless-prototype-1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue9580
gh-Mbed-TLS:archive/20260929T231919Z-rescue/LeonieReichertABB/hpke-functionality
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-hash-no-buffer-copy-1.1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/vanhauser-thc/development
gh-Mbed-TLS:archive/20260929T231919Z-rescue/tpambor/key-wrap
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ruiliio/psa_xts
gh-Mbed-TLS:archive/20261011T063515Z-force/pull/538
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/fix-broken-links-1.1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue669
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/prepare-release-test
gh-Mbed-TLS:archive/20260929T231919Z-rescue/krish2718/wpa3-sae-design-note
gh-Mbed-TLS:archive/20260929T231919Z-rescue/nzzlinh/fix_iar_endianness_1.1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/nzzlinh/fix-iar-endianness
gh-Mbed-TLS:archive/20260929T231919Z-rescue/abtom87/fix-pbkdf2-iter-count
gh-Mbed-TLS:archive/20260929T231919Z-rescue/minosgalanakis/tools/prepare_release_lite
gh-Mbed-TLS:archive/20260929T231919Z-rescue/Harshal5/fix/aes-xts-in-place-cts
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9347-prover-depends-on-9342-9343-9344
gh-Mbed-TLS:archive/20260929T231919Z-rescue/crabel99/async-hardware-support-c35df962
gh-Mbed-TLS:tf-psa-crypto-1.2.0_mergeback
gh-Mbed-TLS:tf-psa-crypto-1.1.1_mergeback
gh-Mbed-TLS:archive/20260929T231919Z-rescue/asmellby/feature/dispatch-library
gh-Mbed-TLS:tf-psa-crypto-release-sync
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ChakshuGupta13/fix/ecdsa-zeroize-stack-key
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/security-md-option-names
gh-Mbed-TLS:archive/20260929T231919Z-rescue/parmi93/patch-2
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/remove-mldsa-sign-from-build
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/tf-m-build-issues
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/test-context-population
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/iar-1.1.0
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/fork-helper-unit-tests-create-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/redundant-declarations
gh-Mbed-TLS:archive/20260929T231919Z-rescue/hasheddan/docs/fix-broken-links
gh-Mbed-TLS:main
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/reintroduce-dh
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/cipher-update-incorrect-error
gh-Mbed-TLS:archive/20260929T231919Z-rescue/Marandil/fix/mbedtls-4314-parse-explicit-curve
gh-Mbed-TLS:archive/20260929T231919Z-rescue/schneider-electric/dev/plaj-se/parse-rsassa-pss
gh-Mbed-TLS:archive/20260929T231919Z-rescue/minosgalanakis/bugfix/program-fuzz-mscv
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daverodgman/aesgcm
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/sha3-coverity-20260224
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bensze01/undef
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue523
gh-Mbed-TLS:archive/20260929T231919Z-rescue/dannytsen/ppc-dev
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daverodgman/aes12
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/ascon-prototype
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daverodgman/chacha10
gh-Mbed-TLS:archive/20260929T231919Z-rescue/kusumitg/implement_psa_pake_set_context_for_spake2p
gh-Mbed-TLS:archive/20260929T231919Z-rescue/davidhorstmann-arm/add-code-size-target
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/simplify-sign_func
gh-Mbed-TLS:archive/20260929T231919Z-rescue/mfil/feature/aes_ni_parallel
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ivv19041994/patch-1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-verify-policy-relax
gh-Mbed-TLS:public_2/main
gh-Mbed-TLS:archive/20260929T231919Z-rescue/mpg/pk-sign-proto
gh-Mbed-TLS:release/tf-psa-crypto1.0.0-documentation
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/0e-plans-july-updates
gh-Mbed-TLS:archive/20260929T231919Z-rescue/davidhorstmann-arm/guard-context-types-in-client-only
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-driver-interface-spec-remove
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/ecp-self-test-race-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/KiruthikPrakash/feature/9342_add-support-for-spake2p-public-key-import-export
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ruiliio/chacha20_16
gh-Mbed-TLS:archive/20260929T231919Z-rescue/irwir/winsock2
gh-Mbed-TLS:archive/20260929T231919Z-rescue/aphroteus/development
gh-Mbed-TLS:archive/20260929T231919Z-rescue/amjoul01/amjoul01/psa-mac-out-of-bound-fix
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/everest-submodule-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/yamt/xtensa-ccount
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/all.sh-generate-only-before-checks-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/Harshal5/feat/builtin_cmac_driver_use_psa_crypto_interface
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/test-data_t-empty_is_empty-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/jean-baptisteboric-eaton/oids-pkcs7-authenticated-attributes
gh-Mbed-TLS:dev/bensze01/public-only-commit
gh-Mbed-TLS:archive/20260929T231919Z-rescue/waleed-elmelegy-arm/psa-key-wrap-aes-kw
gh-Mbed-TLS:archive/20260929T231919Z-rescue/tfloch/psa_add_xts_algorithm
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/msancf-clang16-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/DemiMarie/document-unexpected-tags
gh-Mbed-TLS:archive/20260929T231919Z-rescue/DemiMarie/strict-len
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue9255-tfpsacrypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daantimmer/use-compiler-frontend
gh-Mbed-TLS:archive/20260929T231919Z-rescue/benmcollins/enable-rsassa-pss-pkparse
gh-Mbed-TLS:dev/paul-elliott-arm/add_iop_key_agreement_negative_tests
gh-Mbed-TLS:add-iop-export-pub-key-get-num-ops
gh-Mbed-TLS:dev/rc/new-2-d635ea1
gh-Mbed-TLS:dev/bensze01/jenkins
No reviewers
Labels
Clear labels
api-break
This issue/PR breaks the API and must wait for a new major version
approved
Design and code approved - may be waiting for CI or backports
bug
Something isn't working
component-docs
Docs / website issues filed here for tracking
component-platform
Portability layer and build scripts
component-test
Test framework and CI scripts
dependencies
Pull requests that update a dependency file
deprecation
needs deprecation announcement
DO-NOT-MERGE
enhancement
New feature or request
good first issue
Good for newcomers
help wanted
This issue is not being actively worked on, but PRs welcome.
needs-adoption
stalled PR that someone should pick up and complete
needs-backports
Backports are missing or are pending review and approval.
needs-ci
Needs to pass CI tests
needs-design-approval
Needs design discussion / approval
needs-info
An issue or PR which needs further info from the reporter / author
needs-preceding-pr
Requires another PR to be merged first
needs-review
Every commit must be reviewed by at least two team members
needs-reviewer
This PR needs someone to pick it up for review
needs-work
priority-high
High priority - will be reviewed soon
priority-low
Low priority - this may not receive review soon
priority-medium
Medium priority - this can be reviewed as time permits
priority-scheduled
This PR is big - it will require time to be scheduled for review
priority-very-high
Highest priority - prioritise this over other review work
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.
single-reviewer
This PR qualifies for having only one reviewer
size-l
Estimated task size: large (2w+)
size-m
Estimated task size: medium (~1w)
size-optimisation
size-s
Estimated task size: small (~2d)
size-xs
Estimated task size: extra small (a few hours at most)
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!776
Loading…
Reference in a new issue
No description provided.
Delete branch "bjwtaylor/tf-m-build-issues"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Add fix for memset_s issue discovered with ATFE resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/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
We need to fix an interoperability problem, not remove a feature.
@ -16,2 +16,4 @@#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__)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.
@ -16,2 +16,4 @@#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__)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
github.com/TrustedFirmware-M/trusted-firmware-m@c630c687ae/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.axfThough 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?
@ -16,2 +16,4 @@#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__)Why is there a problem only with AtFE, not with other toolchains?
@ -16,2 +16,4 @@#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__)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
github.com/TrustedFirmware-M/trusted-firmware-m@c630c687ae/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 heregithub.com/TrustedFirmware-M/trusted-firmware-m@c630c687ae/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.@ -16,2 +16,4 @@#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__)@gilles-peskine-arm, I've spoken to TF-M today and their preference is for use to disable the use of the ANNEX K functions in tf-psa-crypto. Though they would also reluctantly add further stubs to the existing
github.com/TrustedFirmware-M/trusted-firmware-m@c630c687ae/bl2/CMakeLists.txt (L162)and here https://github.com/TrustedFirmware-M/trusted-firmware-m/blob/main/bl2/src/mbedcrypto_stubs.c. So I think it comes down to what we want to do, I guess we could have a conditional disable of the ANNEX K functions based on a config? Or we could just say the library requires ANNEX K and tf-m have to stub it, what do you think?@ -16,2 +16,4 @@#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__)TF-PSA-Crypto does not require annex K functions. It uses annex K functions if the system headers declare that annex K is available.
It's perfectly reasonable for TF-M to prefer not to implement
memset_s(). But TF-PSA-Crypto already supports that. The problem is that in one specific build configuration, the TF-M platform (consisting of the TF-M build scripts, the TF-M code and the toolchain code) declares thatmemset_()is available, but does not provide it.Why does this specific combination of project and compiler cause a discrepancy between system headers and actual function availability?
@ -16,2 +16,4 @@#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__)So, thanks of pointing this out, I hadn't realised we had this check. As far as I can tell the reason it fails because the check is a compile time check and the change that tf-m made keeps the compile time logic from atfe, which passes this check. They then change the linker to link in an implementation of libc which doesn't provide this function. So I guess they have further options of fixing the mismatch between compile time and link. Or we could just propose they use MBEDTLS_PLATFORM_ZEROIZE_ALT? Or we could maybe add a link based test, though this would probably not be an ideal for us. What do you think?
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.