Implement md dispatch through PSA (3.2 edition) #6474

gilles-peskine-arm · 2022-10-22T20:40:22Z

In the md module, dispatch via PSA rather than to the built-in hash implementations when a hash is available through a PSA accelerator. This extends the ability to use PSA accelerators to most of the library (everything that's calling md), including TLS, X.509, HMAC_DRBG, deterministic ECDSA, …

This implements #6471, starting from Mbed TLS 3.2.1. I started to implement this from the development version, but it got too complicated due to the hash_info module. It was easier to avoid the hash_info module altogether.

Status: work in progress.

I've only done some basic testing. I expect CI failures because I'm sure I can't have gotten everything right.
This is based on Mbed TLS 3.2.1, so it will have to go through more work before it's ready for development.
I went quickly through the refactorings without thinking about everything, so I probably made some mistakes.
I don't quite understand how some things worked before in a configuration where some hashes are accelerated only, where there's code that has compilation guards on MBEDTLS_SHAxxx_C:
- The hash transcript calculation when MBEDTLS_USE_PSA_CRYPTO is enabled: seems that no hash would be available, yet we can apparently establish TLS connections? (Or maybe we can't, and we just aren't testing a configuration where this is the case.)
- Deterministic ECDSA when called through the PSA API: the HMAC_DRBG code only supported built-in hashes, but the dependencies in the test cases use PSA_WANT_ALG_xxx, so the test cases should be failing?
This is unoptimized. I can think of some ways to save code size, but they'll be material for follow-ups. I don't think this is much worse (maybe even not worse at all) than the current implementation of hash_info if md.c is included, though.

To me, even the current status is enough to demonstrate that dispatching through md is simpler than hash_info, and also that it has similar if not better code size gains possibilities.

Size estimate: S so far, but I suspect it'll grow to M to pass the CI.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

These new symbols will allow code to call the md module and benefit from PSA accelerator drivers. Code must use MBEDTLS_MD_CAN_xxx instead of MBEDTLS_xxx_C to check for support for a particular algorithm. This commit only defines the symbols. Subsequent commits will implement those symbols in the md module, and in users of the md module. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

psa_cipher_encrypt() and psa_cipher_decrypt() sometimes add a zero offset to a null pointer when the cipher does not use an IV. This is undefined behavior, although it works as naively expected on most platforms. This can cause a crash under modern Asan (depending on compiler optimizations). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

When MBEDTLS_MD_xxx_VIA_PSA is enabled (by mbdetls/md.h), route calls to xxx over PSA rather than through the built-in implementation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Avoid repeating the exact logic to choose whether to use PSA or not, and which hashes. In this commit, I have not changed the logic: the choice of hash API is determined by MBEDTLS_USE_PSA_CRYPTO, and the choice of hash algorithm is determined by MBEDTLS_xxx_C (which is wrong when using PSA, since an algorithm could be provied by drivers instead; this will be fixed later). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

With respect to hashes, most modules fall into one of two buckets: the high-level bucket only uses hashes through the md module or through psa, and the low-level bucket only uses hashes through direct calls to a specific sha module. There are a few exceptions where a high-level module makes direct calls to a built-in hash implementation. Annotate those to protect them from a systematic rewrite of hash algorithm dependencies that will enable support of PSA hash implementations via the MD interface. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

These won't work if MD5 is only available via PSA, even when md.c can route via PSA. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In code that call the md module directly or indirectly, use the new MBEDTLS_MD_CAN_xxx symbols instead of MBEDTLS_xxx_C to detect the availability of hash algorithms. This way the code will work even when md routes via PSA, so it will benefit from PSA hash accelerators. ``` perl -i -pe 'next if /notPSA/; s/\bMBEDTLS_(MD[245]|SHA1|SHA[0-9][0-9][0-9]|RIPEMD160)_C\b/MBEDTLS_MD_CAN_$1/g' include/mbedtls/psa_util.h library/md_wrap.h library/psa_crypto_hash.c library/@(ec*|hmac_drbg|oid|pk*|rsa*|ssl*|x509*).[hc] tests/suites/test_suite_@(ec*|hmac_drbg|oid|pk*|rsa*|ssl*|x509*).@(function|data) programs/!(test)/*.c ``` Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2022-10-24T06:21:10Z

include/mbedtls/md.h

+ * - MBEDTLS_MD_SOME_PSA is defined if at least one algorithm is performed
+ *   via PSA.
+ * - MBEDTLS_MD_SOME_LEGACY is defined if at least one algorithm is performed
+ *   via a direct legacy call.
+ *
+ * The md module performs an algorithm via PSA if there is a PSA hash
+ * accelerator, and makes a direct legacy call otherwise.


On second thoughts, I've made this overly complicated. I made it so that dispatch only goes through PSA if there's a driver for the particular algorithm. But why? If there's no driver, PSA just calls the software implementation. So this can be made a lot simpler: either all algorithms dispatch through PSA, or no algorithms dispatch through PSA.

This only impacts the code in md.c and a bit of the setup in md.h, not the modules that use md.

Calling psa_hash_xxx doesn't work in two cases which are currently possible and reasonable (though they won't be possible once PSA is the only interface):

A hash algorithm is implemented in software, but deliberately excluded from the PSA configuration (possible with MBEDTLS_PSA_CRYPTO_CONFIG) because algorithms based on it (e.g. RSA-PSS) are accelerated and the accelerator doesn't support that hash.

The PSA function names make RPC calls to a server (MBEDTLS_PSA_CRYPTO_CLIENT), but there is also some local crypto including hashes (for example, there's hashes plus signature verification plus X.509).

gilles-peskine-arm · 2022-10-24T07:41:15Z

library/psa_crypto.c

@@ -3469,7 +3469,8 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key,
    status = psa_driver_wrapper_cipher_encrypt(
        &attributes, slot->key.data, slot->key.bytes,
        alg, local_iv, default_iv_length, input, input_length,
-        output + default_iv_length, output_size - default_iv_length,
+        ( output == NULL ? NULL : output + default_iv_length ),


I'm sure I've talked with someone about debugging this, but it's not fixed in development. Maybe it's in another pending PR?

We really need to get around running a recent UBSan on the CI.

mpg · 2022-10-24T08:10:38Z

I don't think this is much worse (maybe even not worse at all) than the current implementation of hash_info if md.c is included, though.

One of the points of the current design is so that things work also when md.c is not included, in order to maximize code size saving. So, I think it would be relevant to also compare to the current implementation when md.c is not included.

Also, I'm sorry but I'm not convinced about the priority-high label. I understand you're not convinced by the current design, but I don't think "re-consider the design and possibly redo a significant part of an EPIC from the previous quarter" is something you can single-handedly decide we need to do now. This will remain low on my review list until we had a team discussion about it.

gilles-peskine-arm · 2022-10-24T09:15:11Z

Also, I'm sorry but I'm not convinced about the priority-high label. I understand you're not convinced by the current design, but I don't think "re-consider the design and possibly redo a significant part of an EPIC from the previous quarter" is something you can single-handedly decide we need to do now. This will remain low on my review list until we had a team discussion about it.

I'm not deciding this single-handedly! But we need to have this discussion quickly, because we're about to release an API extension (allowing a bunch of modules to be built without MBEDTLS_MD_C) which isn't particularly useful to users and is costing us in maintenance.

gilles-peskine-arm · 2022-10-24T09:18:49Z

Hmmm, or actually we can still have the minimal glue without MBEDTLS_MD_C. And keep depending on MBEDTLS_MD_C for the larger parts of md.c (algorithm names, HMAC).

mpg · 2022-10-24T09:29:36Z

But we need to have this discussion quickly, because we're about to release an API extension (allowing a bunch of modules to be built without MBEDTLS_MD_C) which isn't particularly useful to users and is costing us in maintenance.

I disagree that it's not particularly useful to users: compiling without MD_C saves code size, and that was a main motivation for the driver-only work.

I also disagree about the urgency: now that we have the ability to extend the config, if in the future we decided to switch back to using md.c we could always auto-enable it in a way that would be transparent to users (except possibly in terms of code size).

mpg · 2022-10-24T09:53:44Z

To put some numbers on the code size claim (on Cortex-M0)

   text    data     bss     dec     hex filename
    236       0       0     236      ec hash_info.o
   1906       0       0    1906     772 md.o

The difference between md.o and hash_info.o is not small, so I think people who care about saving code size when they have drivers are likely to care about the code size savings of removing md.o too.

gilles-peskine-arm · 2022-10-24T11:57:44Z

The difference between md.o and hash_info.o is not small

There are four parts in md:

Hash metadata other than names.
Hash dispatch.
Hash names.
HMAC.

The optimizations I mention earlier include:

Optimizing hash metadata which is currently defined in triplicate (md, hash_info, PSA) — not exactly ×3 in terms of code size because it depends which modules you include, and in PSA they come via macros.
Having some way of not including hash names or HMAC. This would require an extra compile-time option, currently expressed as “MBEDTLS_MD_C disabled”.

It's with those optimizations that I think we can get to roughly the same code size as with hash_info, in builds that don't need hash names or HMAC.

mpg · 2022-10-27T10:56:03Z

I only had a quick look, but I think an important bit that's missing from this prototype/POC is how this impacts other modules in terms of dependency management. That is, what would it take to have something the current component_test_psa_crypto_config_accel_hash_use_psa() (or one of its early incarnations with more things excluded) pass with no more tests skipped than a comparable reference config.

Obviously doing all of it would be too much to ask for a POC/prototype (unless it turns out your approach really makes it so much easier), but I think doing a subset, like PK + RSA (PKCS#1 v1.5 & v2.1), as in #6065 plus #6141, plus a reasonable idea how to generalize, would be a good starting point.

mpg · 2023-02-08T12:21:25Z

Closing as superseded by #6977 (which cherry-picked most of this and re-created the rest).

gilles-peskine-arm · 2023-02-22T21:54:32Z

Actually closing as superseded by #6977.

gilles-peskine-arm added needs-work needs-design-approval component-tls component-crypto Crypto primitives and low-level interfaces size-m Estimated task size: medium (~1w) priority-high High priority - will be reviewed soon labels Oct 22, 2022

gilles-peskine-arm requested a review from mpg October 22, 2022 20:40

gilles-peskine-arm force-pushed the md-psa-3.2 branch 2 times, most recently from de45407 to 2a63a25 Compare October 23, 2022 07:48

gilles-peskine-arm added 12 commits October 23, 2022 09:49

Remove the internal function mbedtls_md_process()

c9e05ff

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Switch metadata functions to the PSA-aware availability symbols

0cdeebc

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix max sizes when PSA is used

61a3e60

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Implement md over PSA

58da9c8

When MBEDTLS_MD_xxx_VIA_PSA is enabled (by mbdetls/md.h), route calls to xxx over PSA rather than through the built-in implementation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

RSA self-test: support SHA1 routed via PSA

8c0dc37

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Remove referenced to hashes not used in TLS

00be181

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Annotate pkparse tests that rely on direct calls to md5.c from pem.c

ed6c993

These won't work if MD5 is only available via PSA, even when md.c can route via PSA. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm commented Oct 24, 2022

View reviewed changes

ronald-cron-arm mentioned this pull request Oct 24, 2022

Architecture: simplify dual-API hash dispatch #6471

Closed

mpg mentioned this pull request Jan 28, 2023

Prototype driver-only hashes via MD #6977

Closed

gilles-peskine-arm closed this Feb 22, 2023

mpg mentioned this pull request Mar 9, 2023

Implement MD dispatch to PSA #7242

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement md dispatch through PSA (3.2 edition) #6474

Implement md dispatch through PSA (3.2 edition) #6474

gilles-peskine-arm commented Oct 22, 2022

gilles-peskine-arm Oct 24, 2022

gilles-peskine-arm Oct 24, 2022

gilles-peskine-arm Oct 24, 2022

mpg commented Oct 24, 2022

gilles-peskine-arm commented Oct 24, 2022

gilles-peskine-arm commented Oct 24, 2022

mpg commented Oct 24, 2022

mpg commented Oct 24, 2022

gilles-peskine-arm commented Oct 24, 2022

mpg commented Oct 27, 2022

mpg commented Feb 8, 2023

gilles-peskine-arm commented Feb 22, 2023

Implement md dispatch through PSA (3.2 edition) #6474

Implement md dispatch through PSA (3.2 edition) #6474

Conversation

gilles-peskine-arm commented Oct 22, 2022

gilles-peskine-arm Oct 24, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Oct 24, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Oct 24, 2022

Choose a reason for hiding this comment

mpg commented Oct 24, 2022

gilles-peskine-arm commented Oct 24, 2022

gilles-peskine-arm commented Oct 24, 2022

mpg commented Oct 24, 2022

mpg commented Oct 24, 2022

gilles-peskine-arm commented Oct 24, 2022

mpg commented Oct 27, 2022

mpg commented Feb 8, 2023

gilles-peskine-arm commented Feb 22, 2023