Some MAX_SIZE macros are too small when PSA ECC is accelerated #7103

valeriosetti · 2023-02-15T10:39:23Z

Description

The goal of this PR is to fix PSA_VENDOR_ECC_MAX_CURVE_BITS when some of the curves are accelerated.
Resolve #6622

~~This PR is a draft because even though ECDH is accelerated there are tests failing due to key derivation. This issue is known and should be addressed in a different PR.~~

~~This PR depends on #7321 (explanation here)~~

Gatekeeper checklist

changelog required as this is a bug fix, provided
backport not required - the bug is only visible when using new features from 3.x (driver-only ECC)
tests provided

include/psa/crypto_sizes.h

valeriosetti · 2023-03-21T11:12:54Z

Notes for reviewers:

I'm finally changing this PR from draft to normal thanks to the latest changes introduced in psa_crypto: Fix psa_key_derivation_output_key ECC without builtin keys #7192
I had to set legacy MBEDTLS_ECP_DP_xxx requirements on some key derivations (see this commit) because:

driver's dispatching is not implemented yet
when deriving weierstrass key it goes through ECP module, which itself uses MBEDTLS_ECP_DP_xxx definitions, which are however excluded from the library part.

BTW the double dependency PSA_WANT_xxx + MBEDTLS_ECP_DP_xxx seems reasonable to me until key derivation does not implement driver's dispatching. @mpg Wdyt?

mpg

Just a first pass.

include/psa/crypto_sizes.h

tests/scripts/all.sh

tests/suites/test_suite_psa_crypto.data

tests/scripts/all.sh

valeriosetti · 2023-03-24T05:22:21Z

@mpg I just set the need-preceding-pr label on this PR because I forgot to mention that it depends on #7321 for a fix that will be introduced by the top level ECC epic; #7321 in particular.

tests/scripts/all.sh

mpg

Looking pretty good, and thanks for adding the 2nd component! Other than the EC J-PAKE depenency, the only things I found are minor nits.

include/mbedtls/check_config.h

tests/scripts/all.sh

mpg · 2023-03-27T09:25:29Z

backport -> question for gatekeepers: since this is a bugfix, should this be backported?

I don't think so. I agree it's technically a bug, but one that only shows in configurations that we don't really support in 2.28, considering that even the possibility of building with driver-only ECDSA or ECDH is new in development. Other reviewers please shout if you disagree.

*tests -> question for gatekeepers: should I test it with outcome-analysis.sh to be sure I did not change the number of tests executed?

I don't think that's needed here, as we didn't massively change existing dependency declarations.

ronald-cron-arm

I only have one minor comment otherwise this looks good to me. Thanks for the two well-crafted non-regression all.sh components. I have checked that when reverting the changes in crypto_size.h the PSA crypto unit tests actually fail in the two components. Otherwise I would say this deserves a change log as it's a bug fix.

ronald-cron-arm · 2023-03-31T12:31:17Z

tests/scripts/all.sh

+
+    # Ensure also RSA_C is disabled so that the size of the public/private
+    # keys cannot be taken from there
+    scripts/config.py unset MBEDTLS_RSA_C


As the PSA configuration is enabled, some PSA_WANT_ALG_RSA_xxx are defined without acceleration this does not have any impact on the build I think.

All RSA associated algs are now forcedly disabled both on library and driver sides. Some PSA driver tests required to be fixed because they were just requiring for not having the built-in version, but they didn't check if the driver one was present (kind of assuming that RSA was always supported on the driver side). Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

mpg

LGTM

mpg · 2023-04-03T08:28:20Z

Despite this being a bug, and sort-of present in 2.28 already, I'm inclined not to backport, because the bug is only visible when using driver-only ECC, which only became possible in 3.4/development. (Practically, in 2.28 we'd have no way to test the fix like we did here.) @valeriosetti @ronald-cron-arm please shout if you disagree.

valeriosetti · 2023-04-03T08:53:33Z

I'm inclined not to backport, because the bug is only visible when using driver-only ECC, which only became possible in 3.4/development

I agree with you. Do I need to write this somewhere (i.e. the fact that we are intentionally not backporting this fix) for future reference?

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

mpg · 2023-04-03T08:59:45Z

Do I need to write this somewhere (i.e. the fact that we are intentionally not backporting this fix) for future reference?

Yes, we want it written in the "gatekeeper checklist" part of the PR description, but no, you don't need to, I had already written it :) Just wanted to make sure you two had an opportunity to tell if you disagreed, because it's easy to not notice I had edited the description.

mpg

LGTM

ronald-cron-arm

LGTM, thanks for the changes.

ronald-cron-arm · 2023-04-03T14:09:00Z

Despite this being a bug, and sort-of present in 2.28 already, I'm inclined not to backport, because the bug is only visible when using driver-only ECC, which only became possible in 3.4/development. (Practically, in 2.28 we'd have no way to test the fix like we did here.) @valeriosetti @ronald-cron-arm please shout if you disagree.

I am fine with that.

Those components were introduced in Mbed-TLS#7103, resolving Mbed-TLS#6622: Some PSA ECC size macros are too small when the largest accelerated curve is larger than the largest built-in curve. At that point, it was not possible yet to omit all built-in curves, so we made these components that had only one (small) curve built-in and all the others accelerated. Now that it's possible to disable all ECC built-ins, and we have tests doing that, we don't need that kind of fiddling any more. Note: these component disabled RSA in order to make sure max key size macros were not taken from RSA. We have test components with all of ECC accelerated and RSA disabled (component_test_psa_crypto_config_accel_ecc_no_bignum and component_test_psa_crypto_config_accel_ecc_ffdh_no_bignum), making the "all curves except one" components really redundant. Note: removing them was one of the items in Mbed-TLS#7757. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

valeriosetti requested a review from mpg February 15, 2023 10:39

valeriosetti self-assigned this Feb 15, 2023

valeriosetti added bug component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first size-s Estimated task size: small (~2d) labels Feb 15, 2023

ronald-cron-arm reviewed Feb 15, 2023

View reviewed changes

include/psa/crypto_sizes.h Outdated Show resolved Hide resolved

ronald-cron-arm self-requested a review February 15, 2023 13:06

valeriosetti mentioned this pull request Mar 17, 2023

psa_crypto: Fix psa_key_derivation_output_key ECC without builtin keys #7192

Merged

3 tasks

valeriosetti force-pushed the issue6622 branch from ffc4534 to f29e760 Compare March 21, 2023 11:07

valeriosetti marked this pull request as ready for review March 21, 2023 11:07

valeriosetti removed the needs-preceding-pr Requires another PR to be merged first label Mar 21, 2023

valeriosetti force-pushed the issue6622 branch from f29e760 to c293e14 Compare March 21, 2023 12:21

valeriosetti mentioned this pull request Mar 22, 2023

Driver-only ECC: all three top-level modules #7321

Merged

3 tasks

mpg reviewed Mar 23, 2023

View reviewed changes

include/psa/crypto_sizes.h Outdated Show resolved Hide resolved

tests/scripts/all.sh Outdated Show resolved Hide resolved

tests/suites/test_suite_psa_crypto.data Outdated Show resolved Hide resolved

mpg reviewed Mar 23, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

mpg reviewed Mar 23, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

mpg reviewed Mar 23, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

valeriosetti force-pushed the issue6622 branch from c293e14 to 3cc9ccd Compare March 24, 2023 05:19

valeriosetti added needs-preceding-pr Requires another PR to be merged first and removed needs-ci Needs to pass CI tests labels Mar 24, 2023

mpg reviewed Mar 24, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

mpg reviewed Mar 24, 2023

View reviewed changes

tests/scripts/all.sh Show resolved Hide resolved

mpg requested changes Mar 27, 2023

View reviewed changes

ronald-cron-arm requested changes Mar 31, 2023

View reviewed changes

valeriosetti dismissed mpg’s stale review via c6ecdad April 3, 2023 06:30

add Changelog

0a7ff79

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti requested review from mpg and ronald-cron-arm April 3, 2023 07:19

mpg previously approved these changes Apr 3, 2023

View reviewed changes

adding missing newline at the end of changelog file

3a3a756

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti dismissed mpg’s stale review via 3a3a756 April 3, 2023 08:55

valeriosetti requested a review from mpg April 3, 2023 08:56

mpg approved these changes Apr 3, 2023

View reviewed changes

ronald-cron-arm approved these changes Apr 3, 2023

View reviewed changes

mpg added the approved Design and code approved - may be waiting for CI or backports label Apr 3, 2023

mpg merged commit 86d5d4b into Mbed-TLS:development Apr 3, 2023

valeriosetti mentioned this pull request Apr 17, 2023

Read and write RFC8410 keys #6838

Merged

3 tasks

valeriosetti deleted the issue6622 branch December 6, 2024 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some MAX_SIZE macros are too small when PSA ECC is accelerated #7103

Some MAX_SIZE macros are too small when PSA ECC is accelerated #7103

valeriosetti commented Feb 15, 2023 •

edited by ronald-cron-arm

Loading

valeriosetti commented Mar 21, 2023 •

edited

Loading

mpg left a comment

valeriosetti commented Mar 24, 2023

mpg left a comment

mpg commented Mar 27, 2023

ronald-cron-arm left a comment •

edited

Loading

ronald-cron-arm Mar 31, 2023

mpg left a comment

mpg commented Apr 3, 2023

valeriosetti commented Apr 3, 2023

mpg commented Apr 3, 2023

mpg left a comment

ronald-cron-arm left a comment

ronald-cron-arm commented Apr 3, 2023

Some MAX_SIZE macros are too small when PSA ECC is accelerated #7103

Some MAX_SIZE macros are too small when PSA ECC is accelerated #7103

Conversation

valeriosetti commented Feb 15, 2023 • edited by ronald-cron-arm Loading

Description

Gatekeeper checklist

valeriosetti commented Mar 21, 2023 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Mar 24, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg commented Mar 27, 2023

ronald-cron-arm left a comment • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Mar 31, 2023

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Apr 3, 2023

valeriosetti commented Apr 3, 2023

mpg commented Apr 3, 2023

mpg left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm commented Apr 3, 2023

valeriosetti commented Feb 15, 2023 •

edited by ronald-cron-arm

Loading

valeriosetti commented Mar 21, 2023 •

edited

Loading

ronald-cron-arm left a comment •

edited

Loading