Implement PSA interruptible sign/verify hash #6737

paul-elliott-arm · 2022-12-06T17:24:52Z

Description

Implement PSA interruptible sign/verify hash, using the internal restartable ECDSA sign / verify interface. Addresses #6332.

Design based on #6279 as of its first commit 0cb0972 . Some requested changes there have been made to this version.

Missing: PSA_WANT guards around the code to remove if functions are not compiled in. I will definitely need some help with this, so this will have to wait until. the new year. I do not believe this prevents review of the rest though, and I hope this will not prove a major amount of work.

Gatekeeper checklist

changelog provided, ~~or not required~~
backport ~~done, or~~ not required
tests provided, ~~or not required~~

gilles-peskine-arm

I've done a design review, not a code review.

Regarding test coverage, the test plan sketch in the issue description might be ok, but it's so lacking details that it's hard to tell what you have in mind. I've added my reflections in comments in the .function file.

Regarding the library, there are major design issues which I think need to be addressed before reviewing. Regarding the conversion to ASN.1, let's not move the code from pk, we really shouldn't be using it. It will be a lot easier to start with a clean history that doesn't move this code back and forth. Regarding compilation guards, we can defer part of the work, but it might make it hard to pass the CI, and even if it passes the CI now it risks hurting driver work so we need to be reasonably clean. Regarding driver support, we can skip it for now as long as what we do doesn't hurt driver support.

include/mbedtls/psa_util.h

library/CMakeLists.txt

library/psa_crypto.c

tests/suites/test_suite_psa_crypto.function

tests/suites/test_suite_psa_crypto.data

tests/suites/test_suite_psa_crypto.function

Signed-off-by: Paul Elliott <[email protected]>

Using PSA interruptible interfaces will cause previously set values to be overwritten. Signed-off-by: Paul Elliott <[email protected]>

Signed-off-by: Paul Elliott <[email protected]>

mpg

I've completed my review, including the new commits, and checked that the feedback from my first review had all been addressed.

I've not found any blockers beyond those raised by Gilles.

Still on my TODO list:

compare API in this PR to that in the API PR;
review lcov results.

mpg · 2023-02-16T08:21:58Z

tests/suites/test_suite_psa_crypto.function

+            *min_completes = 1;
+
+        *max_completes = PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED;
+    } else


[non-blocker] Hmm, shouldn't that be else if( max_ops == PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED )? The function seems to have an undocumented assumption that max_ops will be either unlimited or 0 or 1.

Actually I'd like to suggest an alternative way to structure the function: start with

*min_completes = 1; *max_completes = PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED;

Then handle the cases where we can say something better about one of those bounds (one case for min = 2, one case for max = 1).

This would be changing it back to the way it was prior to extracting it into the function, which Gilles previously wasn't happy with. Github is hiding these conversations from me, so its difficult to refer back to it here, unfortunately.

Yeah, big PRs are intrinsically hard to review, but github is really not making it any easier by hiding stuff like this.

I'm guessing you're refering to this?

I was objecting to

if( max_ops == PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED ) { min_completes = 1; max_completes = 1; } else { min_completes = 2; max_completes = PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED; }

Specifically, setting min_completes = 2 whenever max_ops is finite.

I agree with Manuel that max_completes should be 1 only when max_ops is UNLIMITED.

But now that we have the code in one place, I won't insist on it: it's easy to fix later, and reasonably easy to discover.

tests/suites/test_suite_psa_crypto.function

tests/suites/test_suite_psa_crypto.data

mpg

This is my review of lcov results. It looks like there is a few more cases we could and IMO should test, but I don't think any is a blocker, and they can be left for later if tracked.

library/psa_crypto.c

mpg · 2023-02-16T12:02:53Z

Note: I've compare the API against #6279 and found all the changes to be good. Also, I've checked that all my comments there had been addressed. @gilles-peskine-arm do you want to check that all your comments on #6279 have been addressed here or are you happy to leave that for later?

(I've complete my review, and do not intend to to any more review until something new is pushed.)

paul-elliott-arm · 2023-02-16T18:31:36Z

I believe this is ready for final review now - all blockers have been addressed, and all non-blockers filed as issues.

CI is green, and I have a code-styled and rebased branch ready to go.

gilles-peskine-arm

Unfortunately I found one problem in the latest batch of commits.

Other than that I'm happy with this PR.

library/psa_crypto.c

Previously calling get_num_ops more than once would have ended up with ops getting double counted, and not calling inbetween completes would have ended up with ops getting missed. Fix this by moving this to where the work is actually done, and add tests for double calls to get_num_ops(). Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm · 2023-02-19T23:33:17Z

Hopefully final changes before re-code-style / rebase. Ready for review

mpg

LGTM.

Note (since a rebase is coming): the commit I approved is 34c61f4

mpg · 2023-02-21T10:57:22Z

I rebased and restyled my copy so that I can compare when the same is done with this PR, here are my notes:

I manually rebased on 449bd8303eed8164b83682d2ce028dca0e49b1fa~1 (just before the code style switch). Only one simple add/add conflit to resolve, in include/psa/crypto_values.h (add in INDENT_ON at the end of the list of status codes, where this PR also add the new "non-error" status).
I then rewrote history to apply "Remove #endif from between testcases" as a fixup to "Add State tests" so that no commit make Uncrustify choke.
Then mbedtls-rewrite-branch-style -v was able to run successfully.
The result had no conflicts with development.

gilles-peskine-arm

LGTM

paul-elliott-arm · 2023-02-21T12:26:13Z

The rebase and restyle is being maintained in #7095 - I can either push that here, or we can review / merge that - wdyt @gilles-peskine-arm and @mpg?

Also are we still waiting for @athoelke to review?

gilles-peskine-arm · 2023-02-21T12:47:32Z

The rebase and restyle is being maintained in #7095 - I can either push that here, or we can review / merge that

Whichever you prefer.

mpg · 2023-02-21T12:49:06Z

The rebase and restyle is being maintained in #7095 - I can either push that here, or we can review / merge that

If we're happy with 7095 let's merge that: this will save one round of waiting for the CI.

gilles-peskine-arm · 2023-02-21T14:11:14Z

Superseded by #7095 which is the same semantic content, but rebased and restyled.

paul-elliott-arm self-assigned this Dec 6, 2022

paul-elliott-arm added needs-work needs-ci Needs to pass CI tests component-psa PSA keystore/dispatch layer (storage, drivers, …) size-m Estimated task size: medium (~1w) priority-high High priority - will be reviewed soon and removed needs-work labels Dec 6, 2022

paul-elliott-arm force-pushed the interruptible_sign_hash_internal branch from 764e7b0 to 2e34ee4 Compare December 7, 2022 14:08

gilles-peskine-arm requested changes Dec 7, 2022

View reviewed changes

gilles-peskine-arm added the needs-work label Dec 7, 2022

paul-elliott-arm force-pushed the interruptible_sign_hash_internal branch 3 times, most recently from 7cdcd75 to abf669f Compare December 12, 2022 08:58

mpg self-requested a review December 12, 2022 12:07

paul-elliott-arm force-pushed the interruptible_sign_hash_internal branch 5 times, most recently from 5949478 to 8f5b8ad Compare December 17, 2022 11:07

paul-elliott-arm added 2 commits December 18, 2022 17:47

First draft of PSA interruptible ECC signing design

e38925e

Signed-off-by: Paul Elliott <[email protected]>

Initial empty driver wrapper implementation

be60b06

Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm force-pushed the interruptible_sign_hash_internal branch from 8f5b8ad to d0ec69d Compare December 18, 2022 17:49

paul-elliott-arm mentioned this pull request Dec 18, 2022

PSA Sign Hash (Interruptible) Design #6279

Closed

1 task

PSA level initial implementation

644855f

Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm added 5 commits February 15, 2023 23:53

Add reference to mbedtls_ecp_set_max_ops() to docs

80bda3a

Signed-off-by: Paul Elliott <[email protected]>

Add warning to mbedtls_ecp_set_max_ops()

3dd90c5

Using PSA interruptible interfaces will cause previously set values to be overwritten. Signed-off-by: Paul Elliott <[email protected]>

Add comment to indicate non-PSA spec assertion.

38944d6

Signed-off-by: Paul Elliott <[email protected]>

Remove some abbrevations from test descriptions.

e380115

Signed-off-by: Paul Elliott <[email protected]>

Fix formatting issues

fb60eaa

Signed-off-by: Paul Elliott <[email protected]>

mpg reviewed Feb 16, 2023

View reviewed changes

gilles-peskine-arm requested changes Feb 16, 2023

View reviewed changes

library/psa_crypto.c Outdated Show resolved Hide resolved

library/psa_crypto.c Outdated Show resolved Hide resolved

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 16, 2023

paul-elliott-arm removed the needs-work label Feb 19, 2023

paul-elliott-arm requested review from mpg and gilles-peskine-arm February 19, 2023 23:32

paul-elliott-arm added the needs-review Every commit must be reviewed by at least two team members, label Feb 19, 2023

mpg approved these changes Feb 20, 2023

View reviewed changes

gilles-peskine-arm approved these changes Feb 21, 2023

View reviewed changes

gilles-peskine-arm closed this Feb 21, 2023

gilles-peskine-arm mentioned this pull request Feb 21, 2023

Implement PSA interruptible sign/verify hash (new code style) #7095

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PSA interruptible sign/verify hash #6737

Implement PSA interruptible sign/verify hash #6737

paul-elliott-arm commented Dec 6, 2022 •

edited

Loading

gilles-peskine-arm left a comment

mpg left a comment

mpg Feb 16, 2023

paul-elliott-arm Feb 16, 2023

mpg Feb 16, 2023

gilles-peskine-arm Feb 16, 2023

mpg left a comment

mpg commented Feb 16, 2023

paul-elliott-arm commented Feb 16, 2023

gilles-peskine-arm left a comment

paul-elliott-arm commented Feb 19, 2023

mpg left a comment •

edited

Loading

mpg commented Feb 21, 2023

gilles-peskine-arm left a comment

paul-elliott-arm commented Feb 21, 2023

gilles-peskine-arm commented Feb 21, 2023

mpg commented Feb 21, 2023

gilles-peskine-arm commented Feb 21, 2023

Implement PSA interruptible sign/verify hash #6737

Implement PSA interruptible sign/verify hash #6737

Conversation

paul-elliott-arm commented Dec 6, 2022 • edited Loading

Description

Gatekeeper checklist

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg Feb 16, 2023

Choose a reason for hiding this comment

paul-elliott-arm Feb 16, 2023

Choose a reason for hiding this comment

mpg Feb 16, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Feb 16, 2023

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Feb 16, 2023

paul-elliott-arm commented Feb 16, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm commented Feb 19, 2023

mpg left a comment • edited Loading

Choose a reason for hiding this comment

mpg commented Feb 21, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm commented Feb 21, 2023

gilles-peskine-arm commented Feb 21, 2023

mpg commented Feb 21, 2023

gilles-peskine-arm commented Feb 21, 2023

paul-elliott-arm commented Dec 6, 2022 •

edited

Loading

mpg left a comment •

edited

Loading