Test TF-M config with p256-m driver #8041

mpg · 2023-08-07T11:01:25Z

Description

Resolves #8006.

Note: will need rebasing if #8203 gets merged first.

~~Built on top of~~ Supersedes #8032 ~~itself built on top of #7992 (merged)~~.

One problem that was uncovered by #8032 is that the p256-m driver does not provide all entry points we'd want in order to be able to get rid of ECP_C and BIGNUM_C, namely it's missing the import_key and export_public_key entry points. This PR solve that:

Before adding the entry points, we need to expand the public API in p256-m.h a bit.
Then we add the entry points.
Then add a JSON file, as unlike the entry points that were introduced before, those have programmatic generation of the driver wrappers.
Then a few extra clean-ups in the p256-m driver while at it.

PR checklist

changelog deferred, see Driver-only ECC: BN wrap-up #7757
backport not required - new feature
tests provided

gilles-peskine-arm · 2023-08-07T14:00:02Z

scripts/data_files/driver_jsons/p256_transparent_driver.json

+    "capabilities": [
+        {
+            "mbedtls/c_condition": "defined(MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED)",
+            "entry_points": ["import_key", "export_public_key"],


This isn't the actual list of entry points of the driver. It is, I believe, the list of entry points that are currently supported by the wrapper generator script. That's not very useful, but ok, why not have it, and complete it incrementally. But we really need to explain this, otherwise it's very misleading.

JSON doesn't have comments, but you can put one "_comment": "blah" entry in any JSON object (i.e. a thing between braces) in a driver description.

Addressed in https://github.com/Mbed-TLS/mbedtls/pull/8041/files#diff-e683396c93d3af1ea2db5a3d94788da3916ed084f3eb8be5e5f7c0e429badf39R2820

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

mpg · 2023-09-12T07:45:49Z

Removing "needs-preceding-pr" as #7992 has been merged now, and I'm closing #8032 in favour of this one - Valerio is away so it makes more sense for me to continue here and us to have only one PR to manage.

I'll rebase to resolve conflicts in a moment, before looking at pending comments.

Those will be needed in order for the driver to implement all the PSA key management entry points (currently only implements key generation). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2023-09-20T14:01:38Z

Aw, CI came back red, failing with unexpected discrepancies in "driver test_tfm_config_p256m_driver_accel_ec vs reference test_tfm_config" which is exactly what matters in this PR, so does not look spurious.

The CI was green 3 commits ago and I really didn't expect any of the last 3 commits to break it like this, but here we are... I'll investigate, fix, and let you know when this is ready to re-review.

paul-elliott-arm · 2023-09-20T14:07:23Z

Aw, CI came back red, failing with unexpected discrepancies in "driver test_tfm_config_p256m_driver_accel_ec vs reference test_tfm_config" which is exactly what matters in this PR, so does not look spurious.

The CI was green 3 commits ago and I really didn't expect any of the last 3 commits to break it like this, but here we are... I'll investigate, fix, and let you know when this is ready to re-review.

Looks like a single missing '*' from p256-m_driver_entrypoints.c, line 206 - do you want me to fix this before tomorrow?

mpg · 2023-09-20T14:11:36Z

Ah, right, I was looking at "result analysis" while I should have been looking at all.sh components. I'm fixing this, but thanks for the offer! (Finished early yesterday due to an appointment, making up for it now.)

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2023-09-20T14:15:10Z

Fix pushed, should be ready for review again!

paul-elliott-arm

LGTM, Thanks!

tom-daubney-arm

LGTM, thanks for the changes!

There was a bit of a race condition between Mbed-TLS#8041 which introduced the new entry points, and Mbed-TLS#8203 which documented the list of entry points. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gilles-peskine-arm reviewed Aug 7, 2023

View reviewed changes

mpg force-pushed the tfm-p256m branch 2 times, most recently from ac73ea4 to f8dcacc Compare August 9, 2023 08:41

mpg mentioned this pull request Aug 9, 2023

Test with TF-M config and p256-m driver (part 1) #8032

Closed

3 tasks

mpg 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 priority-high High priority - will be reviewed soon labels Aug 9, 2023

mpg added the needs-preceding-pr Requires another PR to be merged first label Aug 9, 2023

mpg marked this pull request as ready for review August 9, 2023 10:51

mpg changed the title ~~Tfm p256m~~ Test TF-M config with p256-m driver (part 2) Aug 9, 2023

valeriosetti added 7 commits August 11, 2023 08:33

pk: return PK_USE_PSA_EC_DATA to pk.h

d1fba7c

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

test: add component testing TFM configuration and P256M driver

c5c4bd2

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

p256m: minor fixes to the driver interface

983923c

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

test: improve accelerated TFM configuration test and add reference

52ba0e3

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

test: update components' descriptions

ac6d35f

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

analyze_outcome: add new check for parity for TFM configuration

f01d648

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

test: use ASAN flags for testing the accelerated TFM configuration

132240f

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

valeriosetti self-requested a review August 11, 2023 07:42

gilles-peskine-arm added needs-work size-m Estimated task size: medium (~1w) priority-very-high Highest priority - prioritise this over other review work and removed priority-high High priority - will be reviewed soon labels Aug 30, 2023

mpg removed the needs-preceding-pr Requires another PR to be merged first label Sep 12, 2023

mpg changed the title ~~Test TF-M config with p256-m driver (part 2)~~ Test TF-M config with p256-m driver Sep 12, 2023

mpg added 4 commits September 12, 2023 09:50

Add key management utilities to p256-m

bac6d9a

Those will be needed in order for the driver to implement all the PSA key management entry points (currently only implements key generation). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add import_key entry point to p256-m driver

5424cf2

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add export_public_key entry point to p256-m driver

18d7142

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add JSON file for p256-m driver

92a386f

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg requested review from paul-elliott-arm and tom-daubney-arm September 20, 2023 07:47

mpg 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 Sep 20, 2023

Fix typo in variable declaration

3ec976c

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 20, 2023

paul-elliott-arm approved these changes Sep 20, 2023

View reviewed changes

tom-daubney-arm approved these changes Sep 20, 2023

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, labels Sep 20, 2023

mpg removed the needs-ci Needs to pass CI tests label Sep 20, 2023

mpg enabled auto-merge September 20, 2023 15:58

mpg added this pull request to the merge queue Sep 20, 2023

Merged via the queue into Mbed-TLS:development with commit 5edb942 Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test TF-M config with p256-m driver #8041

Test TF-M config with p256-m driver #8041

mpg commented Aug 7, 2023 •

edited

Loading

gilles-peskine-arm Aug 7, 2023

mpg Sep 12, 2023

mpg commented Sep 12, 2023

mpg commented Sep 20, 2023

paul-elliott-arm commented Sep 20, 2023

mpg commented Sep 20, 2023

mpg commented Sep 20, 2023

paul-elliott-arm left a comment

tom-daubney-arm left a comment

Test TF-M config with p256-m driver #8041

Test TF-M config with p256-m driver #8041

Conversation

mpg commented Aug 7, 2023 • edited Loading

Description

PR checklist

gilles-peskine-arm Aug 7, 2023

Choose a reason for hiding this comment

mpg Sep 12, 2023

Choose a reason for hiding this comment

mpg commented Sep 12, 2023

mpg commented Sep 20, 2023

paul-elliott-arm commented Sep 20, 2023

mpg commented Sep 20, 2023

mpg commented Sep 20, 2023

paul-elliott-arm left a comment

Choose a reason for hiding this comment

tom-daubney-arm left a comment

Choose a reason for hiding this comment

mpg commented Aug 7, 2023 •

edited

Loading