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

MbedTLS/TF-M usability improvements for PSA APIs #58023

Merged
merged 3 commits into from
May 26, 2023

Conversation

rajkan01
Copy link

@rajkan01 rajkan01 commented May 18, 2023

Linked PRs

Description

Previously, Zephyr's mbedtls module's cmake build created a single static library, rather than the collection of libraries (mbedtls, mbedcrypto, and mbedx509) that upstream mbedTLS cmake provides.

To give better control at link time to choose the required libraries to link in, this commit updates the Zephyr MbedTLS module to also define three distinct libraries rather than a single static MbedTLS library.

One benefit of the three library approach is that if mbedTLS is used in Zephyr in the the non-secure application in addition to TFM's PSA Crypto API on the secure side with TF-M, PSA API calls on the non-secure side will be redirected to the TFM PSA implementation, and the mbedcrypto library will only be linked to the secure (TF-M) binary, with the mbedtls and mbedx509 libraries linked against the non-secure (Zephyr) binary, enabling TLS calls to PSA crypto to be redirected to mbedcrypto in the secure partition and avoiding function duplication in the non-secure binary.

Additional Changes

Re-commits the psa_crypto sample, which had to be removed during the TF-M 1.7.0 update due to unresolvable linker issues with MbedTLS as a single static library.

Fixes #56995

@zephyrbot zephyrbot added area: TF-M ARM Trusted Firmware-M (TF-M) manifest-mbedtls labels May 18, 2023
@zephyrbot zephyrbot requested a review from joerchan May 18, 2023 09:33
@zephyrbot zephyrbot added manifest manifest-trusted-firmware-m DNM This PR should not be merged (Do Not Merge) labels May 18, 2023
@zephyrbot
Copy link
Collaborator

zephyrbot commented May 18, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
mbedtls zephyrproject-rtos/mbedtls@4e1f66d zephyrproject-rtos/mbedtls@6e7841e (zephyr) zephyrproject-rtos/mbedtls@4e1f66df..6e7841e5
trusted-firmware-m zephyrproject-rtos/trusted-firmware-m@8209cb2 zephyrproject-rtos/trusted-firmware-m@79a6115 (master) zephyrproject-rtos/trusted-firmware-m@8209cb2e..79a6115d

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@carlescufi carlescufi requested a review from tejlmand May 18, 2023 09:49
@rajkan01 rajkan01 force-pushed the psa_crypto_lib_conflicts branch 2 times, most recently from 4c66e9b to 4d36f11 Compare May 18, 2023 11:31
@microbuilder microbuilder added this to the v3.4.0 milestone May 19, 2023
@ceolin
Copy link
Member

ceolin commented May 22, 2023

1c1dd384b877bf6b3fa66bfb9ba85c2528994c5d -> can we make this include path automatic when CONFIG_MBEDTLS is selected ? That is breaking the current usage and and it is not the common pattern on Zephyr AFAIR.

Other than that looks good to me.

@microbuilder
Copy link
Member

1c1dd384b877bf6b3fa66bfb9ba85c2528994c5d -> can we make this include path automatic when CONFIG_MBEDTLS is selected ? That is breaking the current usage and and it is not the common pattern on Zephyr AFAIR.

Other than that looks good to me.

Thanks for the feedback, Flavio. There is a precedent for this here, for example: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/net/lib/sockets/CMakeLists.txt#L35 ... but can you point to an example of what you'd like to see here instead?

@microbuilder
Copy link
Member

@tejlmand Do you have time to look at some of the cmake changes in this patchset, to get these in before feature freeze Friday, and maybe make some suggestions for Raj to update ASAP?

Any chance you're free for the security call today (16:00 in Norway) if we discuss this?

modules/mbedtls/CMakeLists.txt Outdated Show resolved Hide resolved
modules/mbedtls/CMakeLists.txt Outdated Show resolved Hide resolved
@carlescufi carlescufi requested a review from nordicjm May 23, 2023 09:43
modules/mbedtls/CMakeLists.txt Outdated Show resolved Hide resolved
modules/mbedtls/CMakeLists.txt Outdated Show resolved Hide resolved
modules/mbedtls/CMakeLists.txt Show resolved Hide resolved
modules/mbedtls/CMakeLists.txt Show resolved Hide resolved
@carlescufi carlescufi removed the request for review from tejlmand May 23, 2023 10:37
Copy link
Contributor

@joerchan joerchan left a comment

Choose a reason for hiding this comment

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

I want to be stricter in what are exposed in interface include paths.

sha512.h could be define by other modules for example.
mbedtls/sh512.h is very clear.

modules/mbedtls/CMakeLists.txt Outdated Show resolved Hide resolved
modules/mbedtls/CMakeLists.txt Show resolved Hide resolved
@rajkan01
Copy link
Author

rajkan01 commented May 25, 2023

I want to be stricter in what are exposed in interface include paths.

sha512.h could be define by other modules for example. mbedtls/sh512.h is very clear.

@ joerchan I would say this is already taken care of in the mbedTLS sha512.h file if the user defines this flag: https://github.com/zephyrproject-rtos/mbedtls/blob/zephyr/include/mbedtls/sha512.h#L40.

Please let me know if there is any other mbedTLS header file that may have a problem like that.

Rajkumar Kanagaraj added 2 commits May 25, 2023 13:38
Previously, Zephyr's mbedtls module's cmake build created a single static
library, rather than the collection of libraries (mbedtls, mbedcrypto,
and mbedx509) that upstream mbedTLS cmake provides.

To give better control at link time to choose the required libraries to
link, this commit updates the Zephyr MbedTLS module to also define a
collection of libraries rather than a single static MbedTLS library.

One benefit of the three library approach is that if mbedTLS is used in
Zephyr in the the non-secure application in addition to TFM's PSA Crypto
API on the secure side with TF-M, PSA API calls on the non-secure side
will be redirected to the TFM PSA implementation, and the mbedcrypto
library will only be linked to the secure (TF-M) binary, with the mbedtls
and mbedx509 libraries linked against the non-secure (Zephyr) binary,
enabling TLS calls to PSA crypto to be redirected to mbedcrypto in the
secure partition and avoiding function duplication in the non-secure
binary.

Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
Adds a refactored version of the psa_crypto sample back,
which was removed as part of the update to TF-M 1.7.0
due to unresolvable (at the time) issues with use of
MbedTLS instances on the S and NS sides.

This sample takes advantage of changes to MbedTLS and
TF-M that were introduced after the TF-M 1.7.0 and MbedTLS
3.3 release, and cherry-picked in Zephyr, allowing for
improved linking of MbedTLS in secure and non-secure
images. PSA API calls on the non-secure side can now be
correctly routed to the secure partition, while X.509
and TLS calls remain on the non-secure/Zephyr side.

Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
ceolin
ceolin previously approved these changes May 25, 2023
d3zd3z
d3zd3z previously approved these changes May 25, 2023
Copy link
Collaborator

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

The changes look good, but we'll have to wait for the TF-M and mbedtls changes to merge, and update the reference. I'll approve, as there appears to be a DNM on it.

@joerchan
Copy link
Contributor

joerchan commented May 26, 2023

I want to be stricter in what are exposed in interface include paths.
sha512.h could be define by other modules for example. mbedtls/sh512.h is very clear.

@ joerchan I would say this is already taken care of in the mbedTLS sha512.h file if the user defines this flag: https://github.com/zephyrproject-rtos/mbedtls/blob/zephyr/include/mbedtls/sha512.h#L40.

Please let me know if there is any other mbedTLS header file that may have a problem like that.

You misunderstood me, I'm not talking about mbedtls ALT headers for sh512. I'm talking about other modules having their own sh512 header.

In zephyr you can include:
<tinycrypt/sha512.h>
<mbedtls/sha512.h>
If you include:
<sha512.h>
Then which one would you get?

The hostap module (Wi-FI) for example has a sha512.h header file that will be included like "sha512.h". Now I have to make sure that correct sha header file is included by looking at header file path order.

So I don't want "include/mbedtls" and "include/psa" on the include path for this reason.

@rajkan01
Copy link
Author

I want to be stricter in what are exposed in interface include paths.
sha512.h could be define by other modules for example. mbedtls/sh512.h is very clear.

@ joerchan I would say this is already taken care of in the mbedTLS sha512.h file if the user defines this flag: https://github.com/zephyrproject-rtos/mbedtls/blob/zephyr/include/mbedtls/sha512.h#L40.
Please let me know if there is any other mbedTLS header file that may have a problem like that.

You misunderstood me, I'm not talking about mbedtls ALT headers for sh512. I'm talking about other modules having their own sh512 header.

In zephyr you can include: <tinycrypt/sha512.h> <mbedtls/sha512.h> If you include: <sha512.h> Then which one would you get?

The hostap module (Wi-FI) for example has a sha512.h header file that will be included like "sha512.h". Now I have to make sure that correct sha header file is included by looking at header file path order.

So I don't want "include/mbedtls" and "include/psa" on the include path for this reason.

Sorry, as you said, I misunderstood. I made the library include path changes pushed here already; please review and approve if you are okay with the final changes.

joerchan
joerchan previously approved these changes May 26, 2023
@joerchan
Copy link
Contributor

Thanks @rajkan01 Looks good to me now.

Update manifest to fetch latest TFM and MbedTLS.

Signed-off-by: Rajkumar Kanagaraj <rajkumar.kanagaraj@linaro.org>
@rajkan01 rajkan01 dismissed stale reviews from joerchan, d3zd3z, and ceolin via 292107d May 26, 2023 16:20
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label May 26, 2023
Copy link
Collaborator

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Looks good, as long as CI passes.

@nashif nashif merged commit 138faea into zephyrproject-rtos:main May 26, 2023
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mbedtls/tf-m] PSA API conflicts
8 participants