Add PSA interruptible key generation complete API #9651

waleed-elmelegy-arm · 2024-09-27T15:52:12Z

Description

Partially fixes #9106
Fixes #9643

Add PSA interruptible key generation complete API

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog not required
development PR not required
framework PR not required
3.6 PR not required because: New Feature
2.28 PR not required because: New Feature
tests provided | not required because:

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

Multiple simple commits
- please structure your PR into a series of small commits, each of which does one thing
Avoid force-push
- please do not force-push to update your PR - just add new commit(s)
See our Guidelines for Contributors for more details about the review process.

tf-psa-crypto/core/psa_crypto.c

tf-psa-crypto/core/psa_crypto_ecp.c

tf-psa-crypto/core/psa_crypto_core.h

paul-elliott-arm · 2024-10-20T16:41:21Z

tf-psa-crypto/tests/suites/test_suite_psa_crypto.function

        goto exit;
    }

+    /* Test calling setup() 2 times consecutively will fail. */


Still not entirely sure that this is the best place to put this testing - do we really need to do this for all key types and tests? Would like to hear other opinions.

Signed-off-by: Waleed Elmelegy <[email protected]>

Ecp key data length should not be measured by mbedtls_mpi_size(), as this does not count leading zeros, which are still part of the key. This resulted intermittently in the code attempting to import a wrongly sized key as the first byte was all zero. Signed-off-by: Paul Elliott <[email protected]>

Signed-off-by: Waleed Elmelegy <[email protected]>

Add ecp prefix to internal iop generate key function names to emphasize that the functions are doing eliptic curves keys only and not any other types. Signed-off-by: Waleed Elmelegy <[email protected]>

Signed-off-by: Waleed Elmelegy <[email protected]>

yanesca

LGTM

valeriosetti

Thanks for addressing previous comments. I left 2 new ones, but they are really minor and easy to fix.

valeriosetti · 2024-11-25T15:15:26Z

tf-psa-crypto/drivers/builtin/src/psa_crypto_ecp.h

@@ -175,6 +236,7 @@ psa_status_t mbedtls_psa_ecp_generate_key(
 * \retval #PSA_ERROR_CORRUPTION_DETECTED \emptydescription
 * \retval #PSA_ERROR_INSUFFICIENT_ENTROPY \emptydescription
 */
+


Suggested change

In other entries there's no space between the documentation and the function's prototype, so I think we should follow the same pattern here.

valeriosetti · 2024-11-25T15:23:03Z

tf-psa-crypto/drivers/builtin/src/psa_crypto_ecp.c

+    size_t *key_len)
+{
+    *key_len = 0;
+    psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;


Suggested change

psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;

int status = PSA_ERROR_CORRUPTION_DETECTED;

Sorry for not catching this before, but the type is not strictly correct. Indeed this is used with mbedtls_ecp_gen_privkey and mbedtls_mpi_write_binary that return int and, also, you are using the converting function mbedtls_to_psa_error at the end to change int-->psa_status_t.

The same holds also for mbedtls_psa_ecp_generate_key_iop_setup.

Change internal iop generate key error variable to int instead of psa_status_t since the error variable get passed to mbedtls_to_psa_error() when being returned Signed-off-by: Waleed Elmelegy <[email protected]>

yanesca

LGTM

valeriosetti

Sorry, but I just noticed there's something that IMO still need a small fix. Can you please take a look?
Previous changes are fine to me

valeriosetti · 2024-11-26T10:41:46Z

tf-psa-crypto/core/psa_crypto.c

 {
+#if defined(MBEDTLS_ECP_RESTARTABLE)
+    psa_status_t status;
+    uint8_t key_data[PSA_KEY_EXPORT_ECC_KEY_PAIR_MAX_SIZE(PSA_VENDOR_ECC_MAX_CURVE_BITS)+1] = { 0 };


Sorry, I didn't notice this before, but the +1 here looks like a missing guard issue to me...
The commit that added this says Prevent a warning in case PSA_VENDOR_ECC_MAX_CURVE_BITS is set to 0 and I suspect this was generated in builds where no PSA_WANT_ECC_xxx was defined, right? If that's the case then do we really need to support interruptible EC key generation if no EC curve is supported in PSA?
I think guards should be:

PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE in psa_crypto.c along side defined(MBEDTLS_ECP_RESTARTABLE) in psa_generate_key_iop_setup, psa_generate_key_iop_complete, psa_generate_key_iop_abort.

MBEDTLS_PSA_BUILTIN_KEY_TYPE_ECC_KEY_PAIR_GENERATE in psa_crypto_ecp.c to guard new interruptible functions for key pair generation.

Wdyt?

I agree that the guards eventually should look something like that.

However, guarding this code with PSA_WANT defines is out of scope for this PR and is tracked here: #7029

(The driver despatch code is also out of scope and ideally we would add the PSA_WANT defines once we have that as well.)

Ok, then I will approve this PR, but I will add a comment to #7029 to track that this line should be fixed there.

That is a very good idea, thank you very much.

#7029 is about having a PSA equivalent of MBEDTLS_ECP_RESTARTABLE (which may or may not be separate macros for each algorithm, depending on whether we minimize conceptual complexity or practical complexity in this particular case). Here the code is already guarded by MBEDTLS_ECP_RESTARTABLE so it's a different problem.

At a quick glance at the discussion, and not having read everytihing: maybe we should add something in check_config.h to prevent configurations with MBEDTLS_ECP_RESTARTABLE enabled but not some core of ECC, in particular at least one restartable curve?

Most changes have been addressed and the PR has two approvals.

waleed-elmelegy-arm added DO-NOT-MERGE needs-ci Needs to pass CI tests component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) labels Sep 27, 2024

waleed-elmelegy-arm force-pushed the add-iop-key-gen-complete branch 6 times, most recently from 61dd5e9 to 897a1b8 Compare October 3, 2024 21:25

paul-elliott-arm force-pushed the add-iop-key-gen-complete branch 7 times, most recently from 4759795 to 89f5172 Compare October 10, 2024 13:48

paul-elliott-arm force-pushed the add-iop-key-gen-complete branch from 3faedb8 to 55c2121 Compare October 20, 2024 16:39

paul-elliott-arm previously requested changes Oct 20, 2024

View reviewed changes

waleed-elmelegy-arm force-pushed the add-iop-key-gen-complete branch 2 times, most recently from 6a44d62 to 72f3178 Compare October 22, 2024 10:09

waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review and removed DO-NOT-MERGE needs-ci Needs to pass CI tests labels Oct 22, 2024

gilles-peskine-arm added the priority-high High priority - will be reviewed soon label Oct 28, 2024

yanesca self-requested a review November 6, 2024 10:15

waleed-elmelegy-arm and others added 2 commits November 6, 2024 16:57

Add PSA interruptible key generation complete API

30437e6

Signed-off-by: Waleed Elmelegy <[email protected]>

Use PSA Macros instead of legacy ones in psa_generate_key_iop_complete()

44e0f75

Signed-off-by: Waleed Elmelegy <[email protected]>

waleed-elmelegy-arm dismissed yanesca’s stale review via 44e0f75 November 20, 2024 12:32

waleed-elmelegy-arm requested review from valeriosetti and yanesca November 20, 2024 12:33

Prevent a warning in case PSA_VENDOR_ECC_MAX_CURVE_BITS is set to 0

39d54e1

Signed-off-by: Waleed Elmelegy <[email protected]>

yanesca added needs-work needs-ci Needs to pass CI tests and removed 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 labels Nov 20, 2024

waleed-elmelegy-arm added 3 commits November 21, 2024 14:18

Move internal iop generate key function headers to psa_crypto_ecp.h

2266197

Signed-off-by: Waleed Elmelegy <[email protected]>

Add ecp prefix to internal iop generate key function names

f5a82fd

Add ecp prefix to internal iop generate key function names to emphasize that the functions are doing eliptic curves keys only and not any other types. Signed-off-by: Waleed Elmelegy <[email protected]>

Fix code stayle in iop export public-key function

c57f6fc

Signed-off-by: Waleed Elmelegy <[email protected]>

waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Nov 22, 2024

yanesca previously approved these changes Nov 25, 2024

View reviewed changes

valeriosetti requested changes Nov 25, 2024

View reviewed changes

Change internal iop generate key error variable to int

1630603

Change internal iop generate key error variable to int instead of psa_status_t since the error variable get passed to mbedtls_to_psa_error() when being returned Signed-off-by: Waleed Elmelegy <[email protected]>

waleed-elmelegy-arm dismissed yanesca’s stale review via 1630603 November 25, 2024 16:52

yanesca approved these changes Nov 26, 2024

View reviewed changes

yanesca requested a review from valeriosetti November 26, 2024 09:03

valeriosetti requested changes Nov 26, 2024

View reviewed changes

valeriosetti approved these changes Nov 26, 2024

View reviewed changes

valeriosetti mentioned this pull request Nov 26, 2024

PSA Interruptible operations need guarding with PSA_WANT defines #7029

Open

yanesca enabled auto-merge November 26, 2024 16:12

yanesca added this pull request to the merge queue Nov 26, 2024

Merged via the queue into Mbed-TLS:development with commit 49e6115 Nov 26, 2024
6 checks passed

waleed-elmelegy-arm mentioned this pull request Dec 2, 2024

Add interruptible key generation PSA APIs #9528

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PSA interruptible key generation complete API #9651

Add PSA interruptible key generation complete API #9651

waleed-elmelegy-arm commented Sep 27, 2024 •

edited

Loading

paul-elliott-arm Oct 20, 2024

yanesca left a comment

valeriosetti left a comment

valeriosetti Nov 25, 2024

valeriosetti Nov 25, 2024

yanesca left a comment

valeriosetti left a comment

valeriosetti Nov 26, 2024

yanesca Nov 26, 2024

valeriosetti Nov 26, 2024

yanesca Nov 26, 2024

gilles-peskine-arm Nov 26, 2024

	psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
	int status = PSA_ERROR_CORRUPTION_DETECTED;

Add PSA interruptible key generation complete API #9651

Add PSA interruptible key generation complete API #9651

Conversation

waleed-elmelegy-arm commented Sep 27, 2024 • edited Loading

Description

PR checklist

Notes for the submitter

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waleed-elmelegy-arm commented Sep 27, 2024 •

edited

Loading