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.

Sign up for GitHub

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

Jump to bottom

md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

Open
wants to merge 3 commits into
base: mbedtls-3.6
Choose a base branch
from
Open

md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

wants to merge 3 commits into from
+126 −9

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Sep 13, 2024 •
edited
Loading

Description

Instead of allowing PSA dispatching only when CRYPTO_C is set and some MBEDTLS_PSA_ACCEL_ALG_xxx is set, we enable dispatching when CRYPTO_CLIENT and PSA_WANT_ALG_xxx are set. This makes the feature more useful in cases where the PSA support is provided externally, like for example TF-M in Zephyr.

PR checklist

  • changelog not required
  • development PR can be started only once MD will stabilize in that branch
  • framework PR not required
  • 3.6 PR not required because this is it
  • 2.28 PR not required
  • tests not required because this does not affect the behavior of MD and its features are already tested

@valeriosetti valeriosetti self-assigned this Sep 13, 2024
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) labels Sep 13, 2024
@valeriosetti
Copy link
Contributor Author

@mpg I took the liberty to add you as reviewer because you were the one who improved MD last year, allowing for the PSA dispatch, and we also discussed about this change in Slack. However if you don't have enough review bandwidth for this, please let me know and I'll remove you ;)

@valeriosetti valeriosetti force-pushed the md-psa-dispatch-3.6 branch 2 times, most recently from a011d15 to 51772c4 Compare September 16, 2024 08:55
@valeriosetti
Copy link
Contributor Author

The ABI-API break is expected because struct mbedtls_md_context_t will change its size depending on PSA_WANT_ instead of MBEDTLS_PSA_ACCEL_ symbols.

@valeriosetti valeriosetti force-pushed the md-psa-dispatch-3.6 branch 3 times, most recently from acd4c96 to e43f2e1 Compare September 16, 2024 13:27
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Sep 16, 2024
@valeriosetti
Copy link
Contributor Author

CI green (a part from the ABI-API failure mentioned above), so I think the PR is ready for reviews

@mpg
Copy link
Contributor

mpg commented Sep 17, 2024

  • changelog TBD, but I would like to have some feedback from reviewers about this change before doing it.

IMO not ChangeLog for this, as support for CLIENT && !C is not official yet, and when it will become official it will have a ChangeLog entry with a scope much broader than just this :)

@mpg
Copy link
Contributor

mpg commented Sep 17, 2024

  • development PR not required because, if I'm not wrong, the MD module is internal in development, so the user is not able to make direct usage of it.

The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development. However there might be other changes planned (or already done) in development that would make this moot, I'm not sure. Cc @gilles-peskine-arm

@mpg
Copy link
Contributor

mpg commented Sep 17, 2024

The ABI-API break is expected because struct mbedtls_md_context_t will change its size depending on PSA_WANT_ instead of MBEDTLS_PSA_ACCEL_ symbols.

Aw, I'm afraid this is a blocker. We promise not to change the ABI in LTS branches (which 3.6 now is) unless we can't find another way to fix a security issue. So, changing the size of struct mbedtls_md_context_t in the default configuration is not something we can do in 3.6.

Would there be a way to achieve what you're after while keeping the structure unchanged in the default config, and only changing it in a few specific configs?

@mpg mpg added the needs-work label Sep 17, 2024
mpg
mpg requested changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

#9562 (comment)

@valeriosetti
Copy link
Contributor Author

The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development.

Oh, I see. I based my previous comment on the fact that md.h header is now in tf-psa-crypto/drivers/builtin/include/mbedtls/md.h which looks like a quite "hidden" place not meant to be included by end user's code directly. However I surely missed something, so please apologize for the wrong assumption.

Would there be a way to achieve what you're after while keeping the structure unchanged in the default config, and only changing it in a few specific configs?

I think the easiest way will be to enable PSA dispatching based on PSA_WANTs only in the usual CRYPTO_CLIENT && !CRYPTO_C configuration. This is still relevant for Mbed TLS users like Zephyr, but it does not affect the default configuration. I will try to rework the PR in this direction.

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Sep 17, 2024 •
edited
Loading

The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development.

Oh, I see. I based my previous comment on the fact that md.h header is now in tf-psa-crypto/drivers/builtin/include/mbedtls/md.h which looks like a quite "hidden" place not meant to be included by end user's code directly. However I surely missed something, so please apologize for the wrong assumption.

Currently all of the legacy crypto headers have moved as part of the work to split the repositories. We'll move some of them again as part of the work to evolve the API. md.h will remain public (but without HMAC functionality). Most other crypto headers will become private or unstable.

Sorry about the lack of clarity. We're still working on clarifying what's going to happen and making a plan for it to happen in time.

@valeriosetti
adjust_legacy_crypto: move auto-enabling of CRYPTO_CLIENT when CRYPTO_C
3a33ac7 
Move the auto-enabling of CRYPTO_CLIENT when CRYPTO_C at the
beginning of the file so that all that becomes later is aware
of this.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled
13b5ea8 
Instead of allowing PSA dispatching only when CRYPTO_C is set and
some MBEDTLS_PSA_ACCEL_ALG_xxx is set, we enable dispatching
when CRYPTO_CLIENT and PSA_WANT_ALG_xxx are set. This makes
the feature more useful in cases where the PSA support is
provided externally, like for example TF-M in Zephyr.

This commit also:
- fixes disparities in hmac accel/ref comparison;
- add proper stubs for the PSA crypto build without a PSA
  provider (CRYPTO_CLIENT && !CRYPTO_C scenario);
- add proper guards for tests trying to use MD+PSA dispatch.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
adjust_legacy_crypto: improve enablement of MBEDTLS_MD_xxx_VIA_PSA
de2f6f9 
The previous change that replaced CRYPTO_C with CRYPTO_CLIENT
caused an increase of the mbedtls_md struct in scenarios where
the hash related PSA_WANTs were enabled, but not accelerated.
This caused an ABI-API break which is not allowed for an LTS
branch.
Since the main goal here is to allow PSA dispatch in a "pure
crypto client" scenario, we partially revert the previous change
to config_adjust_legacy_crypto.h and add an extra condition
for "CRYPTO_CLIENT && !CRYPTO_C".

This commit also reverts changes done in analyze_outcomes.py
because they are no more necessary.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Sep 17, 2024 •
edited
Loading

Would there be a way to achieve what you're after while keeping the structure unchanged in the default config, and only changing it in a few specific configs?

I think the easiest way will be to enable PSA dispatching based on PSA_WANTs only in the usual CRYPTO_CLIENT && !CRYPTO_C configuration. This is still relevant for Mbed TLS users like Zephyr, but it does not affect the default configuration. I will try to rework the PR in this direction.

Thanks to this change the ABI-API failure disappeared and the CI is fully green now :)

The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development.

Oh, I see. I based my previous comment on the fact that md.h header is now in tf-psa-crypto/drivers/builtin/include/mbedtls/md.h which looks like a quite "hidden" place not meant to be included by end user's code directly. However I surely missed something, so please apologize for the wrong assumption.

Currently all of the legacy crypto headers have moved as part of the work to split the repositories. We'll move some of them again as part of the work to evolve the API. #8450 (but without HMAC functionality). #8663.

Thanks a lot for the update! So I think that the forward port of this fix will need to wait until MD design is stabilized/planned on development.

mpg
mpg approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees

@valeriosetti valeriosetti

Labels
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 size-xs Estimated task size: extra small (a few hours at most)
Projects
Roadmap Board for Mbed TLS
Status: In Review
Roadmap pull requests (new board)
Status: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@valeriosetti @mpg @gilles-peskine-arm