Add PSA interruptible key generation setup & abort APIs #9639

waleed-elmelegy-arm · 2024-09-25T16:52:39Z

Description

Partially fixes #9106
fixes #9642
Add PSA interruptible key generation setup & abort APIs

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 because:
development PR not required because:
framework PR not required
3.6 PR not required because: New feature
2.28 PR pnot required because: New feature
tests provided

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.

waleed-elmelegy-arm · 2024-10-02T17:04:20Z

CI failure not related

paul-elliott-arm

Seems generally ok, just would like an answer to the copying key attributes question.

paul-elliott-arm · 2024-10-18T17:09:26Z

tf-psa-crypto/core/psa_crypto.c

+        goto exit;
+    }
+
+


Nit: Extraneous line

paul-elliott-arm · 2024-10-18T17:10:29Z

tf-psa-crypto/core/psa_crypto.c

+        goto exit;
+    }
+
+    operation->attributes = *attributes;


Question: is this really the best way to copy attributes? I admittedly can't find any other operations copying attributes, but this feels dangerous.

There are no pointers in the struct currently, there might be an issue if someone adds a pointer in the future and it soesn't seem very propable to add a pointer to attributes, if we decide that it is still dangerous what I can do is add copy_attributes() function that still does a shallow copy and later will do a deep copy if needed.

Good question!

There used to be a pointer in the attributes (domain parameters). We removed it in 3.6.0. I don't recall that we ever copied the data at that pointer: either we never copied attributes anywhere (most likely — we used to copy a sub-structure that didn't have pointers), or we only copied attributes at a point where we knew there were no domain parameters, or we had a memory management bug.

At this point I can't think of anything in the proposed evolution of the PSA API that would make us add a pointer back. But just in case it would be good to write and use a copy function, that can be just a static inline that does an assignment.

Actually there is one place where we are copying the attribute structure: in psa_start_key_creation. We used to copy a sub-structure (attributes.core) that didn't have any embedded pointer, and we flattened the structure when we removed the pointer.

So at this point it's a preexisting issue. (Of my doing, incidentally — I did the patch that flattened the structure.) It would be good to use a copy function for future-proofing. But we would also need to ensure that we're calling psa_key_attributes_reset on all paths where the attributes become dead. We used to do that, but one of the advantages of simplifying the attribute structure was that we wouldn't have to do it any longer.

So at this point, I lean weakly towards not bothering. If we ever add a pointer into the attribute structure, we'll have to review the code carefully to ensure that all places that might possibly copy or free an attribute structure are covered by the copy/free function, anyway. We won't gain much just because there are places where the copy/free functions are used, since there'll be no way to know that it's missing in other places.

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

yanesca

I have a couple of questions and suggestions which I left inline.

One generic remark: it is easier to review a PR when it is divided into smaller commits. This one for example could have been 3 commits. (I am not asking you to split it up, this is just a remark/ask for future PRs which haven't been raised yet.)

tf-psa-crypto/core/psa_crypto.c

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

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

tf-psa-crypto/include/psa/crypto_struct.h

tf-psa-crypto/core/psa_crypto_core.h

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

yanesca

Looks good to me.

If I am not missing something obvious here, it would be nice to add that, but I won't hold off the review for that.

yanesca · 2024-11-01T16:36:31Z

(API break CI failure is necessary and acceptable.)

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

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

…in iop key generation" This reverts commit 005b78c. Signed-off-by: Waleed Elmelegy <[email protected]>

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

waleed-elmelegy-arm · 2024-11-04T17:29:37Z

Had to force push because the revert commit lacked signing off

yanesca · 2024-11-05T09:23:02Z

Error: Test case not executed: test_suite_psa_crypto;PSA generate key: ECC, SECP224K1, good
Error: Test case not executed: test_suite_psa_crypto;PSA generate key: ECC, brainpool160r1, good

Both of these curves are scheduled to be removed in 4.0, I think it is acceptable to remove these two test cases to have a clean CI.

Remove generate key brainpool160r1 & SECP224K1 test cases as they are scheduled to be removed in 4.0 . Signed-off-by: Waleed Elmelegy <[email protected]>

yanesca

LGTM

paul-elliott-arm

LGTM

yanesca · 2024-11-06T09:10:48Z

(API break CI failure is necessary and acceptable.)

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 25, 2024

waleed-elmelegy-arm force-pushed the add-iop-key-gen-setup branch 6 times, most recently from 989ca52 to 6ba71ec Compare September 27, 2024 14:12

waleed-elmelegy-arm added 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 and removed DO-NOT-MERGE needs-ci Needs to pass CI tests labels Sep 27, 2024

waleed-elmelegy-arm requested review from paul-elliott-arm and gilles-peskine-arm September 27, 2024 17:31

paul-elliott-arm reviewed Oct 18, 2024

View reviewed changes

Add PSA interruptible key generation setup & abort APIs

cca4dbe

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

waleed-elmelegy-arm force-pushed the add-iop-key-gen-setup branch from 6ba71ec to cca4dbe Compare October 21, 2024 12:42

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

yanesca requested changes Oct 28, 2024

View reviewed changes

yanesca added needs-work 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 Oct 29, 2024

waleed-elmelegy-arm added 5 commits October 31, 2024 16:35

Add Testing interuptible key generation initialization

1c3c5b1

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

Remove mention of drivers for interuptible key generation

75a412f

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

Improve interuptible key generation testing

e3abcc3

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

Remove useless define check for interuptible key agreement APIs

bb06832

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

Improve comments for interuptible key agreement APIs

b16edbe

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 labels Oct 31, 2024

yanesca previously approved these changes Nov 1, 2024

View reviewed changes

yanesca requested a review from paul-elliott-arm November 1, 2024 16:33

Add testing valid attributes after aborting an invalid input in iop k…

005b78c

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

waleed-elmelegy-arm dismissed yanesca’s stale review via 005b78c November 1, 2024 17:09

yanesca reviewed Nov 4, 2024

View reviewed changes

tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved

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, labels Nov 4, 2024

waleed-elmelegy-arm added 2 commits November 4, 2024 17:28

Revert "Add testing valid attributes after aborting an invalid input …

1f5075b

…in iop key generation" This reverts commit 005b78c. Signed-off-by: Waleed Elmelegy <[email protected]>

Add extra tests cases for different Weierstrass curves for key genera…

df186be

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

waleed-elmelegy-arm force-pushed the add-iop-key-gen-setup branch from 2766f10 to df186be Compare November 4, 2024 17:29

Remove generate key brainpool160r1 & SECP224K1 test cases

1ea62b1

Remove generate key brainpool160r1 & SECP224K1 test cases as they are scheduled to be removed in 4.0 . Signed-off-by: Waleed Elmelegy <[email protected]>

yanesca approved these changes Nov 5, 2024

View reviewed changes

yanesca added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 5, 2024

paul-elliott-arm approved these changes Nov 5, 2024

View reviewed changes

paul-elliott-arm 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, needs-ci Needs to pass CI tests labels Nov 5, 2024

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

Merged via the queue into Mbed-TLS:development with commit 502ff7b Nov 6, 2024
4 of 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 setup & abort APIs #9639

Add PSA interruptible key generation setup & abort APIs #9639

waleed-elmelegy-arm commented Sep 25, 2024 •

edited

Loading

waleed-elmelegy-arm commented Oct 2, 2024

paul-elliott-arm left a comment

paul-elliott-arm Oct 18, 2024

paul-elliott-arm Oct 18, 2024

waleed-elmelegy-arm Oct 21, 2024

gilles-peskine-arm Oct 21, 2024

gilles-peskine-arm Oct 22, 2024

yanesca left a comment

yanesca left a comment

yanesca commented Nov 1, 2024

waleed-elmelegy-arm commented Nov 4, 2024

yanesca commented Nov 5, 2024

yanesca left a comment

paul-elliott-arm left a comment

yanesca commented Nov 6, 2024

Add PSA interruptible key generation setup & abort APIs #9639

Add PSA interruptible key generation setup & abort APIs #9639

Conversation

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

Description

PR checklist

Notes for the submitter

waleed-elmelegy-arm commented Oct 2, 2024

paul-elliott-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm Oct 18, 2024

Choose a reason for hiding this comment

paul-elliott-arm Oct 18, 2024

Choose a reason for hiding this comment

waleed-elmelegy-arm Oct 21, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Oct 21, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Oct 22, 2024

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

yanesca commented Nov 1, 2024

waleed-elmelegy-arm commented Nov 4, 2024

yanesca commented Nov 5, 2024

yanesca left a comment

Choose a reason for hiding this comment

paul-elliott-arm left a comment

Choose a reason for hiding this comment

yanesca commented Nov 6, 2024

waleed-elmelegy-arm commented Sep 25, 2024 •

edited

Loading