Skip to content

[tf-psa-crypto] md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled#188

Merged
mpg merged 7 commits intoMbed-TLS:developmentfrom
valeriosetti:md-psa-dispatch-tfpsacrypto
Mar 4, 2025
Merged

[tf-psa-crypto] md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled#188
mpg merged 7 commits intoMbed-TLS:developmentfrom
valeriosetti:md-psa-dispatch-tfpsacrypto

Conversation

@valeriosetti
Copy link
Copy Markdown
Contributor

@valeriosetti valeriosetti commented Feb 28, 2025

Description

This is the forward porting of Mbed-TLS/mbedtls#9562.
I'm opening the PR here instead of the main repo because MD was moved in this one.

I (manually 1) copied all the commits from Mbed-TLS/mbedtls#9562 a part:

PR checklist

Footnotes

  1. because I didn't find a way to make git to help me with this (I tried cherry-pick, format-patch + am, diff + apply).

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 <vsetti@baylibre.com>
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 add proper guards for tests trying to use MD+PSA
dispatch.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti added size-s Estimated task size: small (~2d) needs-review Every commit must be reviewed by at least two team members priority-high High priority - will be reviewed soon needs-ci Needs to pass CI tests labels Feb 28, 2025
@valeriosetti valeriosetti self-assigned this Feb 28, 2025
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM apart from the missing changelog and the spurious macros.

However, I missed something, since the client-server tests are failing — we need to implement psa_can_do_hash there.

#if defined(MBEDTLS_PSA_CRYPTO_C)

#if defined(MBEDTLS_PSA_ACCEL_ALG_MD5)
#define MBEDTLS_MD_CAN_MD5
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MBEDTLS_MD_CAN_xxx no longer exists in TF-PSA-Crypto.

(Ok, it's technically harmless to define it, but it's confusing, since it won't be used.)

Comment thread core/psa_crypto_core.h
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members labels Feb 28, 2025
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 <vsetti@baylibre.com>
This allows any implementer of the PSA client interface to easily
include this header and therefore function's prototype.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti force-pushed the md-psa-dispatch-tfpsacrypto branch from 459c101 to c50d397 Compare March 3, 2025 14:30
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members and removed needs-work labels Mar 3, 2025
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg
Copy link
Copy Markdown
Contributor

mpg commented Mar 4, 2025

The only CI failure is fixed by the companion Mbed TLS PR Mbed-TLS/mbedtls#10027 so it is acceptable.

@mpg mpg removed the needs-ci Needs to pass CI tests label Mar 4, 2025
Copy link
Copy Markdown
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!

Comment thread drivers/builtin/include/mbedtls/config_adjust_legacy_crypto.h
@github-project-automation github-project-automation Bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Mar 4, 2025
@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members labels Mar 4, 2025
@mpg mpg merged commit 2574203 into Mbed-TLS:development Mar 4, 2025
@github-project-automation github-project-automation Bot moved this from Has Approval to Done in Roadmap pull requests (new board) Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Development

Successfully merging this pull request may close these issues.

3 participants