Add multipart cipher accelerator support and test driver #3644

stevew817 · 2020-09-03T13:41:35Z

Description

This PR adds support for delegating multipart cipher operations to an accelerator driver complying to the spec in #3493. It also adds a multipart cipher test driver implementing AES-CTR, as requested in #3345. Test suite tests are added to make sure the test driver is called when expected.

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

NOTE: this PR currently depends on #3501 (reflected in the fact that it is based on the head of 3501). Will rebase once 3501 gets merged to development, so for review, please only look at commits e6167ca .. b9eddca.

Todos

Tests
Documentation
Changelog updated

Steps to test or reproduce

Tests are added to the test suite.

stevew817 · 2020-09-04T07:16:52Z

@danh-arm Can you tell me what the internal CI is complaining about this time?

gilles-peskine-arm · 2020-09-04T07:56:57Z

The only CI failure is the ABI check, which signals that some structures in crypto_struct.h have changed size. So we need to bump the libmbedcrypto soversion. It doesn't have to be done in this PR: we usually do it as part of the release. So you can consider the CI to have passed.

ronald-cron-arm · 2020-09-07T16:50:15Z

I've had a quick look to the commits and overall it looks as I was expecting (following the review of #3501) with regard to changes in psa_crypto.c, psa_crypto_driver_wrappers.c and the testing. Please rebase on top of development and update the changes according to the review of #3501 and I will do the complete review.

stevew817 · 2020-09-07T19:03:21Z

Awesome, thanks. Will do tomorrow.

gilles-peskine-arm · 2020-09-07T19:51:15Z

Like Ronald, I've had a quick look and the general shape looks about right. I would suggest one design change: rather than a “cipher” driver, I think it makes more sense to have a “multipart” driver, which would be extended later with multipart hash, MAC and AEAD.

stevew817 · 2020-09-08T11:21:54Z

rather than a “cipher” driver, I think it makes more sense to have a “multipart” driver, which would be extended later with multipart hash, MAC and AEAD.

Not sure I agree. I'm more convinced these should be grouped by functional (algorithm) group.

stevew817 · 2020-09-08T13:38:34Z

@ronald-cron-arm and @gilles-peskine-arm, I've now rebased, so actual review can go ahead.

include/psa/crypto_struct.h

library/psa_crypto.c

tests/src/drivers/cipher.c

tests/suites/test_suite_psa_crypto_driver_wrappers.data

tests/include/test/drivers/cipher.h

tests/suites/test_suite_psa_crypto_driver_wrappers.function

include/psa/crypto_struct.h

library/psa_crypto.c

library/psa_crypto_driver_wrappers.c

library/psa_crypto.c

tests/suites/test_suite_psa_crypto_driver_wrappers.function

ronald-cron-arm · 2020-09-09T07:00:14Z

tests/src/drivers/cipher.c

+    (void) iv;
+    (void) iv_size;
+    (void) iv_length;
+


Ditto, please add the "usual" code for negative testing.

ronald-cron-arm · 2020-09-09T07:01:30Z

tests/src/drivers/cipher.c

+    (void) operation;
+    (void) iv;
+    (void) iv_length;
+


ditto, code for negative testing.

ronald-cron-arm · 2020-09-09T07:12:59Z

tests/suites/test_suite_psa_crypto_driver_wrappers.data


 generate_key through transparent driver: error
 generate_key:PSA_ERROR_GENERIC_ERROR:"":PSA_ERROR_GENERIC_ERROR
+


Please add some negative testing as well as you did in #3501. We should have for each driver entry point at least one test where the driver entry point returns in error.

stevew817 · 2020-09-09T16:13:56Z

@gilles-peskine-arm and @ronald-cron-arm , your review comments should now be incorporated in the changes. The one exception is negative testing, which I'll add, but I'd like you to review the changes already while I work on the negative testcases.

gilles-peskine-arm · 2020-09-10T08:29:17Z

The pr-merge job passed except for the expected ABI change in crypto_struct.h so CI can be considered a pass. The pr-head job had a failure due to a network glitch but it's unnecessary to run it since pr-merge passed.

gilles-peskine-arm

Mostly looking good, but a few issues remain.

library/psa_crypto.c

include/psa/crypto_struct.h

library/psa_crypto.c

tests/src/drivers/cipher.c

stevew817 · 2020-09-10T13:10:33Z

@gilles-peskine-arm thanks for the continued feedback. All issues you flagged should now be resolved.

stevew817 · 2020-09-10T16:08:56Z

@ronald-cron-arm negative testing is now implemented too.

ronald-cron-arm · 2020-09-10T16:18:19Z

ok, thanks, looking.

ronald-cron-arm

Thank you very much for all the changes especially the addition of negative testing. Here is my second round of review, mostly suggestions for improvement.

library/psa_crypto.c

tests/suites/test_suite_psa_crypto_driver_wrappers.function

tests/suites/test_suite_psa_crypto_driver_wrappers.data

stevew817 · 2020-09-11T09:46:32Z

@ronald-cron-arm Your feedback has been taken into account. Does this look good to you now?

ronald-cron-arm · 2020-09-11T10:10:37Z

Thanks. I am attending a training right know. I will look at it this afternoon.

ronald-cron-arm

Thanks again for the changes. The code in psa_crypto.c is easier to read for me with the mbedtls_in_use flag rather than the driver_in_use flag. I am happy with the tests now. Still a few small things in psa_crypto.c though.

library/psa_crypto.c

stevew817 · 2020-09-14T11:35:55Z

@ronald-cron-arm Final cosmetic changes done per your request

Once an operation has been 'accepted' by a driver, the remainder is bound to the same driver, since driver-specific context structs cannot be shared. This provides a pretty good gate mechanism for the fallback logic, too. Signed-off-by: Steven Cooreman <[email protected]>

Signed-off-by: Steven Cooreman <[email protected]>

Added zeroization of the wrapper context on failure/abort, and reliance on the crypto core to not call an uninitialised wrapper. Signed-off-by: Steven Cooreman <[email protected]>

Signed-off-by: Steven Cooreman <[email protected]>

As pointed out by Ronald. The key slot is populated using get_key_from_slot, and after calling the driver the slot is validated to not contain an external key, so calling get_transparent_key is superfluous. Signed-off-by: Steven Cooreman <[email protected]>

As pointed out by Gilles Signed-off-by: Steven Cooreman <[email protected]>

Signed-off-by: Steven Cooreman <[email protected]>

* Reworked the cipher context once again to be more robustly defined * Removed redundant memset * Unified behaviour on failure between driver and software in cipher_finish * Cipher test driver setup function now also returns early when its status is overridden, like the other test driver functions * Removed redundant test cases * Added bad-order checking to verify the driver doesn't get called where the spec says it won't. Signed-off-by: Steven Cooreman <[email protected]>

Signed-off-by: Steven Cooreman <[email protected]>

stevew817 · 2020-09-14T15:11:28Z

Rebased onto development due to merge conflict after merge of #3480. @ronald-cron-arm and @gilles-peskine-arm, please re-approve.

gilles-peskine-arm

I reviewed the commits up to 4d55104, then I checked that merging this with development gives the same content as 6d81f7e (up to cosmetic choices).

Looks good to me.

CI passes. The failure is the API/ABI check pointing out the expected ABI changes in crypto_struct.h.

ronald-cron-arm

I've been through the rebase myself and ended-up with a result compatible with the result in this PR. LGTM.

ronald-cron-arm self-requested a review September 7, 2020 16:43

ronald-cron-arm mentioned this pull request Sep 8, 2020

Implemented a test driver for opaque signatures without storage as defined in issue #3343. #3526

Closed

2 tasks

stevew817 force-pushed the feature/transparent_multipart_driver branch from 5436492 to e3fa23d Compare September 8, 2020 13:37

gilles-peskine-arm self-requested a review September 8, 2020 15:39

gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces HwDrivers needs-review Every commit must be reviewed by at least two team members, labels Sep 8, 2020

gilles-peskine-arm added this to the September 2020 Sprint milestone Sep 8, 2020

gilles-peskine-arm requested changes Sep 8, 2020

View reviewed changes

gilles-peskine-arm reviewed Sep 8, 2020

View reviewed changes

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

gilles-peskine-arm added the needs-work label Sep 8, 2020

ronald-cron-arm requested changes Sep 9, 2020

View reviewed changes

gilles-peskine-arm requested changes Sep 10, 2020

View reviewed changes

ronald-cron-arm requested changes Sep 11, 2020

View reviewed changes

library/psa_crypto.c Show resolved Hide resolved

library/psa_crypto.c Outdated Show resolved Hide resolved

library/psa_crypto.c Outdated Show resolved Hide resolved

stevew817 added 21 commits September 14, 2020 16:31

Update license texts

3ec4018

Signed-off-by: Steven Cooreman <[email protected]>

Add a working implementation of a multipart AES-CTR test driver

8b12225

Signed-off-by: Steven Cooreman <[email protected]>

Exercise the AES-CTR multipart driver and check it gets called

d53c1bf

Signed-off-by: Steven Cooreman <[email protected]>

Add changelog entry

a32193f

Signed-off-by: Steven Cooreman <[email protected]>

Add new files to IDE project

435beea

Signed-off-by: Steven Cooreman <[email protected]>

Structify cipher test driver hook variables

acb5a10

Signed-off-by: Steven Cooreman <[email protected]>

Style and language fixes from review

5240e8b

Signed-off-by: Steven Cooreman <[email protected]>

Only pass the driver-relevant portion of the context struct

fb81aa5

Signed-off-by: Steven Cooreman <[email protected]>

Simplify cipher setup wrapper body

150c99b

Signed-off-by: Steven Cooreman <[email protected]>

Unify driver context struct handling

cfeea8f

Added zeroization of the wrapper context on failure/abort, and reliance on the crypto core to not call an uninitialised wrapper. Signed-off-by: Steven Cooreman <[email protected]>

Check & set operation flags for both driver and software in the core

7df0292

Signed-off-by: Steven Cooreman <[email protected]>

Unify cipher setup function inside test driver

16afd3d

Signed-off-by: Steven Cooreman <[email protected]>

Make sure to not call mbedtls_cipher_free on an uninitialised context

e55f3e8

As pointed out by Gilles Signed-off-by: Steven Cooreman <[email protected]>

Implement one-shot cipher in test driver

fe0ab55

Signed-off-by: Steven Cooreman <[email protected]>

Add testing of fallback to cipher operations

eca33c6

Signed-off-by: Steven Cooreman <[email protected]>

Update semantics around driver context use in psa_cipher_setup

a07b997

Signed-off-by: Steven Cooreman <[email protected]>

Add mock and negative testing to cipher driver

89e54f2

Signed-off-by: Steven Cooreman <[email protected]>

Final feedback from @ronald-cron-arm

6d81f7e

Signed-off-by: Steven Cooreman <[email protected]>

stevew817 dismissed ronald-cron-arm’s stale review via 6d81f7e September 14, 2020 15:10

stevew817 force-pushed the feature/transparent_multipart_driver branch from 4d55104 to 6d81f7e Compare September 14, 2020 15:10

gilles-peskine-arm approved these changes Sep 14, 2020

View reviewed changes

gilles-peskine-arm requested a review from ronald-cron-arm September 14, 2020 17:43

ronald-cron-arm approved these changes Sep 15, 2020

View reviewed changes

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Sep 15, 2020

gilles-peskine-arm merged commit 7107e66 into Mbed-TLS:development Sep 15, 2020

stevew817 mentioned this pull request Mar 3, 2021

Dispatch hash operations through the driver wrapper layer #4157

Merged

2 tasks


		generate_key through transparent driver: error
		generate_key:PSA_ERROR_GENERIC_ERROR:"":PSA_ERROR_GENERIC_ERROR

Add multipart cipher accelerator support and test driver #3644

Add multipart cipher accelerator support and test driver #3644

Uh oh!

Conversation

stevew817 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

Uh oh!

stevew817 commented Sep 4, 2020

Uh oh!

gilles-peskine-arm commented Sep 4, 2020

Uh oh!

ronald-cron-arm commented Sep 7, 2020

Uh oh!

stevew817 commented Sep 7, 2020

Uh oh!

gilles-peskine-arm commented Sep 7, 2020

Uh oh!

stevew817 commented Sep 8, 2020

Uh oh!

stevew817 commented Sep 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronald-cron-arm Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

stevew817 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

stevew817 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

stevew817 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

stevew817 commented Sep 9, 2020

Uh oh!

gilles-peskine-arm commented Sep 10, 2020

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevew817 commented Sep 10, 2020

Uh oh!

stevew817 commented Sep 10, 2020

stevew817 commented Sep 3, 2020 •

edited

Loading

ronald-cron-arm left a comment •

edited

Loading