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: some enhancement for PSA crypto core #80136

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

valeriosetti
Copy link
Collaborator

@valeriosetti valeriosetti commented Oct 21, 2024

  • Enable legacy CONFIG_MBEDTLS_CIPHER_AES_ENABLED when CONFIG_PSA_WANT_KEY_TYPE_AES is set and Mbed TLS is the PSA crypto API provider (i.e. CONFIG_MBEDTLS_PSA_CRYPTO_C). This mimic what happens in Mbed TLS at build time.
  • Enable CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG instead of CONFIG_MBEDTLS_PSA_CRYPTO_LEGACY_RNG if the platform has any CSPRNG enabled. This helps reducing RAM/ROM footprint.

ceolin
ceolin previously approved these changes Nov 3, 2024
jhedberg
jhedberg previously approved these changes Nov 3, 2024
@dkalowsk
Copy link
Contributor

dkalowsk commented Nov 4, 2024

As this has a migration guide for 4.0 change, I'm guessing this is desired to be in 4.0. Given that we're at RC2 now, it's going to need a GitHub Issue to make that happen. I'm 90% sure this can just be linked to the deprecation of TinyCrypt but I'll leave that up to you :-) If you think this should be part of 4.0 please add the Milestone.

@valeriosetti
Copy link
Collaborator Author

As this has a migration guide for 4.0 change, I'm guessing this is desired to be in 4.0. Given that we're at RC2 now, it's going to need a GitHub Issue to make that happen. I'm 90% sure this can just be linked to the deprecation of TinyCrypt but I'll leave that up to you :-) If you think this should be part of 4.0 please add the Milestone.

This is only required for #79931 which is still drafted, so no this won't be part of 4.0. I'll move the documentation

@valeriosetti valeriosetti dismissed stale reviews from jhedberg and ceolin via 4ef6d7c November 5, 2024 14:04
@valeriosetti valeriosetti force-pushed the psa-fix-for-bt branch 4 times, most recently from fd5c53f to 9dc4b33 Compare November 5, 2024 17:56
@dkalowsk
Copy link
Contributor

dkalowsk commented Nov 6, 2024

As discussed in the Release meeting today, we want to consider the deprecation of Tinycrypt a release blocker item for 4.0. That would mean Part1, Part2, and the mbedTLS changes would all need to be merged in. This is one of the changes identified for inclusion.

@ceolin any objection to this course of action? The point was raised that Tinycrypt has been "just about to be deprecated" for 5 years now, but never seems to make it. We'd like to make it happen now.

Auto-select MBEDTLS_CIPHER_AES_ENABLED when AES support is requested
through PSA (i.e. CONFIG_PSA_WANT_KEY_TYPE_AES) and the PSA support is
provided through Mbed TLS itself (i.e. CONFIG_MBEDTLS_PSA_CRYPTO_C).

This mimic what happens in Mbed TLS at build time: if AES support
is required through PSA, but there's no one else providing it
(i.e. no TF-M in Zephyr) then provide this support through legacy
AES module.

This is useful in samples/tests so that the user can simply use the
PSA_WANT symbol to ask for AES support in PSA crypto and then tune
the AES features (ex: CONFIG_MBEDTLS_AES_ROM_TABLES) without the need
to also define CONFIG_MBEDTLS_CIPHER_AES_ENABLED.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
The main problem of MBEDTLS_PSA_CRYPTO_LEGACY_RNG is that it
brings in some legacy modules (entropy + ctr_drbg/hmac_drbg)
which means extra ROM/RAM footprint.
MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG instead simply calls to the
CSPRNG which makes it definitely smaller.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
As long as MBEDTLS_ENTROPY_C is enabled, Mbed TLS needs to
poll some entropy source to gather data that will then be
processed by CTR/HMAC-DRBG modules. This means that in most
of the cases, once MBEDTLS_ENTROPY_C is enabled then also
MBEDTLS_ENTROPY_POLL_ZEPHYR needs to be enabled. This was
done manually until now, as the long list of samples/tests
demonstrate.

This commit solves this dependency by defaulting
MBEDTLS_ENTROPY_POLL_ZEPHYR to on as soon as
MBEDTLS_ENTROPY_C is set. As a consequence, all manual
enablement of MBEDTLS_ENTROPY_POLL_ZEPHYR in samples/tests
are removed.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@tomi-font tomi-font added this to the v4.0.0 milestone Nov 6, 2024
Comment on lines +79 to +80
is set), then the Kconfig option :kconfig:option:`CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG`
is the default choice for random number source instead of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
is set), then the Kconfig option :kconfig:option:`CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG`
is the default choice for random number source instead of
is set), the Kconfig option :kconfig:option:`CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG`
is now the default choice for the random number source instead of

@tomi-font
Copy link
Collaborator

please review @ceolin

Copy link
Collaborator

@frkv frkv left a comment

Choose a reason for hiding this comment

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

Good enhancements! Hope this gets in quickly

@mmahadevan108 mmahadevan108 modified the milestones: v4.0.0, v4.1.0 Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants