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

Patch: Align MbedTLS PSA headers with TFM PSA headers #43

Merged
merged 4 commits into from
May 26, 2023

Conversation

rajkan01
Copy link

@rajkan01 rajkan01 commented May 16, 2023

Description

Reason for the patch:

Pending a future update to MbedTLS 3.4 and TF-M 1.8.0, the ARM TF-M and MbedTLS team recommends the below patch that allows the MbedTLS PSA headers to align with TFM PSA headers: Mbed-TLS/mbedtls#7151

  • If MBEDTLS_PSA_CRYPTO_C is enabled, we always enable MBEDTLS_PSA_CRYPTO_CLIENT, since the client-side functions are part of the full PSA crypto feature set. Historically, we didn't have a good place for configuration modification, so we did this early in the crypto.h include tree. Since Mbed TLS 3.0, we have mbedtls/build_info.h for that.

  • Integrators of Mbed TLS may override the header files psa/crypto_platform.h and psa/crypto_struct.h by overwriting the files or by placing alternative versions earlier in the include file search path. These two methods are sometimes inconvenient, so allow a third method which doesn't require overwriting files or having a precise order for the include path: integrators can now specify alternative names for the headers.

  • Test that MBEDTLS_PSA_CRYPTO_PLATFORM_FILE and MBEDTLS_PSA_CRYPTO_STRUCT_FILE can be set to files in a directory that comes after the standard directory in the include file search path.

  • Before, if psa/crypto_platform.h was overridden and the override didn't include mbedtls/build_info.h, it was possible to end up with parts of the headers not taking the library configuration into account, if no mbedtls header was included before psa/crypto.h. Make sure that the mbedtls configuration is visible from the start, no matter what is or is not in the platform header.

Gatekeeper checklist

  • changelog provided, or not required
  • backport done, or not required
  • tests provided, or not required

@ceolin
Copy link
Member

ceolin commented May 17, 2023

No complains with the changes, they make sense for me.

Integrators of Mbed TLS may override the header files
"psa/crypto_platform.h" and "psa/crypto_struct.h" by overwriting the files
or by placing alternative versions earlier in the include file search path.
These two methods are sometimes inconvenient, so allow a third method which
doesn't require overwriting files or having a precise order for the include
path: integrators can now specify alternative names for the headers.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
(cherry picked from commit b1176f2)
Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
Test that MBEDTLS_PSA_CRYPTO_PLATFORM_FILE and
MBEDTLS_PSA_CRYPTO_STRUCT_FILE can be set to files in a directory that comes
after the standard directory in the include file search path.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
(cherry picked from commit df6e84a)
Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
Before, if psa/crypto_platform.h was overridden and the override didn't
include "mbedtls/build_info.h", it was possible to end up with parts of
the headers not taking the library configuration into account, if no
mbedtls header was included before "psa/crypto.h". Make sure that
the mbedtls configuration is visible from the start, no matter what is
or is not in the platform header.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
(cherry picked from commit 361b5f9)
Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
If MBEDTLS_PSA_CRYPTO_C is enabled, we always enable
MBEDTLS_PSA_CRYPTO_CLIENT, since the client-side functions are part of the
full PSA crypto feature set. Historically, we didn't have a good place for
configuration modification, so we did this early in the crypto.h include
tree. Since Mbed TLS 3.0, we have mbedtls/build_info.h for that.

Addresses Mbed-TLS/mbedtls#7144 .

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
(cherry picked from commit 95c9152)
Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
@ceolin
Copy link
Member

ceolin commented May 22, 2023

+1 for the patches but aren't they included in the v3.4 ? Shouldn't we just bump mbedTLS to this newer release instead of cherry picking these commits ?

@microbuilder
Copy link
Member

microbuilder commented May 22, 2023

+1 for the patches but aren't they included in the v3.4 ? Shouldn't we just bump mbedTLS to this newer release instead of cherry picking these commits ?

I had hoped to update to TF-M 1.8.0 which also used MbedTLS 3.4.0, but it will take more time than we have before the feature freeze Friday to fully test, etc., so we were planning to update to both at the start of the next release.

Sorting out the issues with the MbedTLS library outputs and PSA API linking between Secure and Non-Secure with upstream MbedTLS and TF-M took longer than expected.

This is still a good first step to improving the situation with TF-M integration, though, and we have the next release cycle to sort out remaining issues, and further decouple the projects when TF-M isn't being used.

@d3zd3z d3zd3z merged commit 6e7841e into zephyrproject-rtos:zephyr May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants