Implement MD dispatch to PSA #7242

mpg · 2023-03-09T16:13:00Z

Description

Resolves #6990

See https://github.com/Mbed-TLS/mbedtls/blob/development/docs/architecture/psa-migration/md-cipher-dispatch.md#md-light specifically sub-sections "MD algorithm support macros" to "Error code conversion". (The first subsection was #7120.)

The first few commits just port the implementation from Gilles's original prototype, with a few related additions (for example, handle size macros at the same time as utility functions). Then changes between the original prototype and the final design document (that is, the dynamic aspect of dispatch) are implemented.

Gatekeeper checklist

changelog not required - will be added at the end of the series
backport not required - new feature
tests provided

Please ignore while reviewing this PR. Will be removed by rebasing after Mbed-TLS#7242 is merged. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mprse

Changes and tests looks good. Left 2 comments with for places that needs clarification.

mprse · 2023-03-13T10:19:53Z

tests/include/test/psa_crypto_helpers.h

+#if !defined(MBEDTLS_PSA_CRYPTO_C)
+#define PSA_INIT() ((void) 0)
+#define PSA_DONE() ((void) 0)
+#endif /* MBEDTLS_PSA_CRYPTO_C */
+


I'm not sure if this is correct at least needs clarification. We are overwriting macros that are defined earlier in the header and are used by other macros.

I think we should have something like this:

#if defined(MBEDTLS_PSA_CRYPTO_C) #define PSA_INIT() PSA_ASSERT(psa_crypto_init()) #else #define PSA_INIT() ((void) 0) #endif

I think in builds without MBEDTLS_PSA_CRYPTO_C, we're not overwriting anything as those macros have not been defined before. (Evidence: before I added that, I got "undeclared function" warnings, and also if we were re-defining the macros I think we'd get warnings about it.)

But I take your point that the way it's done is not clear enough and I agree that what you're suggesting is much clearer. I'll rework that.

tests/suites/test_suite_md.function

AndrzejKurek

Some comments left.
Also: Can the decision on where to use PSA_INIT() in tests and where not be somehow justified, explained, or highlighted? What I mean is, we have a couple of tests in test_suite_md (and in fact in other test suites using md explicitly or under the hood too) that will now dispatch the calls to different places based on PSA_INIT(). Should we identify these places to find if there are any that specifically shouldn't use PSA_INIT()? Starting from test_suite_md itself, tests like md_text, md_hex - why shouldn't these be run with PSA_INIT()?

AndrzejKurek · 2023-03-13T13:05:14Z

library/md.c

+
+static int mbedtls_md_error_from_psa(psa_status_t status)
+{
+    switch (status) {


This could use/adapt the newly merged https://github.com/Mbed-TLS/mbedtls/pull/6829/files#diff-ca89f54f744763de9f77c774fa7854f1f7ae92ade1e99676329cf161248e87b2R37
, as shown for example here.

Thanks for the info, I didn't noticed this had been merged! If that's OK with you, I'd like to leave that for the "size optimization" part - I've added a note about it to an existing issue: #6993 (comment)

AndrzejKurek · 2023-03-13T13:10:37Z

library/md.c

+    }
+}
+
+static int md_uses_psa(const mbedtls_md_info_t *info)


Isn't this more like md_can_use_psa? Even it this returns 1, but the engine is set to legacy, a given info will not use psa, right?

AndrzejKurek · 2023-03-13T14:08:37Z

library/psa_crypto.c

@@ -7108,6 +7115,13 @@ psa_status_t psa_crypto_init(void)
        return PSA_SUCCESS;
    }

+    /* Init drivers */
+    status = psa_driver_wrapper_init();


https://github.com/Mbed-TLS/mbedtls/blob/development/docs/architecture/psa-migration/strategy.md mentions that
There is a problem with the modules used for the PSA RNG, as currently the RNG is initialized before drivers and the key store.
I'm not sure if this part should be updated, or maybe some studies have been done on impact (and it'll be good to link them here), but it seems to me that this might have some additional effects.

I think this part should be updated - it probably should have been worded "The might be a problem" because IIRC it's not that we were sure there was a problem, more that there was something to do and we weren't sure it wouldn't cause issues. Now it's done and have not been causing issues.

Ah, that's in strategy.md, not md-cipher-dispatch.md. In that case, if you don't mind I'd rather fix that as part of #7156 because there's potentially a lot more that's out-of-date in that file right now. I've left a note.

AndrzejKurek · 2023-03-13T14:12:16Z

library/psa_crypto.c

@@ -7108,6 +7115,13 @@ psa_status_t psa_crypto_init(void)
        return PSA_SUCCESS;
    }

+    /* Init drivers */
+    status = psa_driver_wrapper_init();


Do we want to check if the drivers were initialized to not initialize them again? Documentation on psa_driver_wrapper_init is quite scarce.

Currently initializing just the drivers is not supported (psa_driver_wrapper_init() is a private function), the only thing that's supported is initializing everything with psa_crypto_init() which already has a guard, so I think that's OK.

(Note: #6636 will change that but it's not in yet.)

What's the gain of introducing drivers_initialized then? I assumed that it should support partial psa_crypto_init success:

If psa_crypto_init succeeds, then indeed, global_data.initialized is equal to 1 and there's no need to check drivers_initialized;

If psa_crypto_init fails after psa_driver_wrapper_init was called succesfully then the state is inconsistent and we could prevent a second call to drivers that were already initialized, during a second call to psa_crypto_init;

If that's not the point and we don't want to support partial initialization, then why add drivers_initialized at all?

Ah, excellent question! Indeed in the context of this PR there's not point yet, I'm just anticipating on #7155 where the entropy module will start using MD, with the goal of having it work when hashes are provided by drivers. The entropy module is first used at the "init RNG" stage in psa_crypto_init(), so when it will call MD, global_data.initialized will still be 0 (it's only changed to 1 at the end of the function), but we'll want MD to dispatch to drivers nonetheless (which is also the reason for the change of ordering).

Now that you mention it, my PR structure is not clean: in this PR indeed I shouldn't have changed the order or introduced drivers_initialized, but instead use global_data.initialized in psa_can_do_hash(), and left the rest for the PR about the entropy module. I'm afraid I got a bit ahead of myself here.

Ah, now it makes sense! Thanks for explaining. I still think that we could optimize to not call init on drivers if they are already initialized, but it's a minor.

AndrzejKurek · 2023-03-13T14:13:58Z

include/mbedtls/md.h

+
+/*
+ * - MBEDTLS_MD_CAN_xxx is defined if the md module can perform xxx.
+ * - MBEDTLS_MD_xxx_VIA_PSA is defined if the md module performs xxx via PSA.


This is a bit misleading, as the md module will still perform xxx via legacy if the engine states that it should.

mpg · 2023-03-14T10:12:39Z

@mprse @AndrzejKurek Thanks for your reviews! I think I've addressed all of your feedback, either by changes in the code, or making a note to address it later if that's OK with you, or in one instance explaining why I think no change's needed.

Can the decision on where to use PSA_INIT() in tests and where not be somehow justified, explained, or highlighted?

Ah, I think I just added PSA_INIT() when a test would otherwise fail, which is a very weak rationale now that you make me spell it out :) Indeed when thinking about it, all tests that call md_setup() or do an actual computation now need PSA_INIT() at least in some configurations. I've now added that in a systematic way, thanks for pointing it out.

AndrzejKurek · 2023-03-14T11:16:46Z

tests/suites/test_suite_md.function

+#define MD_PSA_INIT()   PSA_INIT()
+#define MD_PSA_DONE()   PSA_DONE()
+#else /* MBEDTLS_MD_SOME_PSA */
+#define PSA_INIT() ((void) 0)


Suggested change

#define PSA_INIT() ((void) 0)

#define MD_PSA_INIT() ((void) 0)

Without this change (and line below) the code will not compile out of the box.

AndrzejKurek · 2023-03-14T11:19:19Z

include/mbedtls/md.h

@@ -36,7 +36,8 @@

 /*
 * - MBEDTLS_MD_CAN_xxx is defined if the md module can perform xxx.
- * - MBEDTLS_MD_xxx_VIA_PSA is defined if the md module performs xxx via PSA.
+ * - MBEDTLS_MD_xxx_VIA_PSA is defined if the md module performs xxx via PSA
+ *   (when PSA Crypto is initialized).


Minor: this applies to all lines below, or maybe it would be just better to say that "PSA can perform xxx", just as with MD above.

AndrzejKurek · 2023-03-14T11:44:38Z

I've now added that in a systematic way, thanks for pointing it out.

What about other test suites that call it indirectly? ECJPAKE, pkcs12, x509write, ct, RSA (in rsaes_oaep, which aparently isn't tested in test_suite_rsa), and ssl all use md. Do we want to also make sure that PSA_INIT is called in these tests? Or do we assume that these should be implementation-agnostic and work in any available way?

mpg · 2023-03-14T11:54:46Z

I've now added that in a systematic way, thanks for pointing it out.

What about other test suites that call it indirectly? ECJPAKE, pkcs12, x509write, ct, RSA (in rsaes_oaep, which aparently isn't tested in test_suite_rsa), and ssl all use md. Do we want to also make sure that PSA_INIT is called in these tests? Or do we assume that these should be implementation-agnostic and work in any available way?

I was thinking about that. I think for now they don't need PSA_INIT because they're only supposed to work when either hashes are available in software or (1) for PK, X.509, TLS: USE_PSA_CRYPTO is defined, (2) for the others: MD_C is not defined. Of course the ultimate goal is that these will start working in more circumstances, at which point the calls to PSA_INIT will start being necessary. I'd say that will happen in #6991 when we replace the existing constraining macros from legacy_or_psa.h with MBEDTLS_MD_CAN and stop doing the #if defined(MD_C) /* use MD */ #else /* use PSA */ #endif dance in mixed-domain modules such as RSA.

Note: RSA-OAEP is tested in test_suite_pkcs1_v21 I think. FWIW I agree it's confusing, and this should be renamed to test_suite_rsa_pkcs1_v21 at least, to show it's related to RSA.

mpg · 2023-03-14T11:57:41Z

I've labelled "do not merge" as I'd rather wait until after the code freeze for the release to merge this. This is not really complete without #6991, and while I think breaking the work in multiple issues makes sense to avoid overly large PRs, I think in a release it's better to either not have this PR, or have #6991 to go with it.

AndrzejKurek

Looks good given the constraints discussed in comments and planned follow-up work.

For multi-part operations, we want to make the decision to use PSA or not only once, during setup(), and remember it afterwards. This supports the introduction, in the next few commits, of a dynamic component to that decision: has the PSA driver sub-system been initialized yet? Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

This will be used in the next commit. While at it, move driver initialization before RNG init - this will be handy when the entropy module wants to use drivers for hashes. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

We shouldn't dispatch to PSA when drivers have not been initialized yet. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

- Support for PSA_CRYPTO_CLIENT without PSA_CRYPTO_C is out of scope for now but might be added later (the architecture supports that). - While we're using a void pointer for md_ctx, we don't need a union here; the union will be useful only if & when we remove the indirection. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

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

The test driver library tries to only build what's necessary, but must use the same PSA_WANT macros as the main library. So, for things that are not needed, it undefines MBEDTLS_PSA_BUILTIN_xxx and defines MBEDTLS_PSA_ACCEL_xxx, unless the ACCEL symbol was defined on the command line, in which case it undefines it and defineds BUILTIN instead. This negation happens in crypto_config_test_driver_extension.h and reflects the fact that what we want accelerated in the main library is what we want built-in in the driver library (and vice versa if we want to minimize the size of the driver library). So, the ACCEL symbols in inside the test driver library (while it's being built, not those on the command line) are a bit of a white lie: they don't actually mean "there's an accelerator for this" but instead "I won't include a built-in for this even though the corresponding PSA_WANT symbol is defined". This was quite harmless until MD started making dispatch decisions based on the ACCEL symbols: when it tries to dispatch to an accelerator that doesn't actually exist, things tend to go badly. The minimal fix for this is to change how we enable extra hashes in the test driver library: by defining the ACCEL symbol on the command line, in the build we'll end up with the BUILTIN symbol (and implementation!) and no ACCEL symbol, which is exactly what we want. Long version: https://arm-ce.slack.com/archives/GTM3SM1K5/p1675071671707599 Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

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

All tests that call md_setup() or compute a hash of a HMAC may now need it in some builds. 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-03-16T08:54:33Z

Rebased and force-pushed to resolve the conflict (a trivial one: development had whitespace changes in the definition of PSA_DONE() which I'm moving to another place in the same file here).

valeriosetti

LGTM!
I only found one possible suggestion about the mbedtls_md_error_from_psa() function, but then I saw it was already discussed and found a plan for it, so I'm OK with this PR.
Just wait for the CI to be OK as well.

AndrzejKurek · 2023-03-16T14:16:46Z

tests/suites/test_suite_md.function

@@ -114,6 +128,8 @@ void md_info(int md_type, char *md_name, int md_size)
    (void) md_name;
 #endif

+    /* Note: PSA Crypto init not needed to info functions */


Very minor:

Suggested change

/* Note: PSA Crypto init not needed to info functions */

/* Note: PSA Crypto init not needed for info functions */

I'll fix that in a follow-up in order to avoid another round of CI here.

mpg self-assigned this Mar 9, 2023

mpg added needs-ci Needs to pass CI tests 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 Mar 9, 2023

mpg added the priority-high High priority - will be reviewed soon label Mar 10, 2023

mprse self-requested a review March 10, 2023 10:19

mpg removed the needs-ci Needs to pass CI tests label Mar 13, 2023

mpg mentioned this pull request Mar 13, 2023

Use md light #7271

Merged

3 tasks

AndrzejKurek self-requested a review March 13, 2023 10:22

AndrzejKurek removed the needs-reviewer This PR needs someone to pick it up for review label Mar 13, 2023

mprse reviewed Mar 13, 2023

View reviewed changes

AndrzejKurek suggested changes Mar 13, 2023

View reviewed changes

mpg mentioned this pull request Mar 14, 2023

Update strategy.md for MD dispatch #7156

Closed

mpg requested review from mprse and AndrzejKurek March 14, 2023 10:12

AndrzejKurek reviewed Mar 14, 2023

View reviewed changes

mpg force-pushed the md-dispatch-psa branch from 1f35277 to a120e91 Compare March 14, 2023 11:20

mpg mentioned this pull request Mar 14, 2023

MD: optimize the case with no built-in hash #7277

Open

mpg mentioned this pull request Mar 14, 2023

Move most things to MD_CAN macros #6991

Closed

mpg added the DO-NOT-MERGE label Mar 14, 2023

AndrzejKurek previously approved these changes Mar 14, 2023

View reviewed changes

mpg added 12 commits March 16, 2023 09:46

Dispatch according to init status.

9b14639

We shouldn't dispatch to PSA when drivers have not been initialized yet. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Finish removing HMAC from MD-light

39a376a

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

Fix failures in builds without PSA_CRYPTO_C

7dc8b95

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

Clarify real/dummy def of PSA_INIT/DONE

9f132b7

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

Rename internal function to something clearer

f48b1f8

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

Clarify a comment

a9ab4a2

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

Systematically call PSA_INIT for MD tests

ec31f29

All tests that call md_setup() or compute a hash of a HMAC may now need it in some builds. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Further clarify a comment

9d698df

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

mpg dismissed AndrzejKurek’s stale review via 9d698df March 16, 2023 08:52

mpg force-pushed the md-dispatch-psa branch from 834dcfd to 9d698df Compare March 16, 2023 08:52

mpg removed the DO-NOT-MERGE label Mar 16, 2023

mpg requested a review from AndrzejKurek March 16, 2023 08:54

valeriosetti self-requested a review March 16, 2023 09:15

valeriosetti approved these changes Mar 16, 2023

View reviewed changes

AndrzejKurek approved these changes Mar 16, 2023

View reviewed changes

AndrzejKurek 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 Mar 16, 2023

mpg removed the needs-ci Needs to pass CI tests label Mar 17, 2023

mpg removed the request for review from mprse March 17, 2023 08:42

mpg merged commit ec000c1 into Mbed-TLS:development Mar 17, 2023

mpg mentioned this pull request Apr 6, 2023

PBKDF2: implement output_bytes() (HMAC) #7132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement MD dispatch to PSA #7242

Implement MD dispatch to PSA #7242

mpg commented Mar 9, 2023 •

edited

Loading

mprse left a comment

mprse Mar 13, 2023

mpg Mar 13, 2023

AndrzejKurek left a comment

AndrzejKurek Mar 13, 2023

mpg Mar 14, 2023

AndrzejKurek Mar 13, 2023

AndrzejKurek Mar 13, 2023

mpg Mar 14, 2023

mpg Mar 14, 2023

AndrzejKurek Mar 13, 2023 •

edited

Loading

mpg Mar 14, 2023

AndrzejKurek Mar 14, 2023 •

edited

Loading

mpg Mar 14, 2023

AndrzejKurek Mar 14, 2023

AndrzejKurek Mar 13, 2023

mpg commented Mar 14, 2023

AndrzejKurek Mar 14, 2023 •

edited

Loading

AndrzejKurek Mar 14, 2023

AndrzejKurek commented Mar 14, 2023 •

edited

Loading

mpg commented Mar 14, 2023

mpg commented Mar 14, 2023

AndrzejKurek left a comment

mpg commented Mar 16, 2023

valeriosetti left a comment

AndrzejKurek Mar 16, 2023

mpg Mar 17, 2023

mpg Mar 17, 2023

	#define PSA_INIT() ((void) 0)
	#define MD_PSA_INIT() ((void) 0)

	/* Note: PSA Crypto init not needed to info functions */
	/* Note: PSA Crypto init not needed for info functions */

Implement MD dispatch to PSA #7242

Implement MD dispatch to PSA #7242

Conversation

mpg commented Mar 9, 2023 • edited Loading

Description

Gatekeeper checklist

mprse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek 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

Choose a reason for hiding this comment

AndrzejKurek Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek Mar 14, 2023 • edited Loading

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

mpg commented Mar 14, 2023

AndrzejKurek Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek commented Mar 14, 2023 • edited Loading

mpg commented Mar 14, 2023

mpg commented Mar 14, 2023

AndrzejKurek left a comment

Choose a reason for hiding this comment

mpg commented Mar 16, 2023

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

mpg commented Mar 9, 2023 •

edited

Loading

AndrzejKurek Mar 13, 2023 •

edited

Loading

AndrzejKurek Mar 14, 2023 •

edited

Loading

AndrzejKurek Mar 14, 2023 •

edited

Loading

AndrzejKurek commented Mar 14, 2023 •

edited

Loading