Dispatch hash operations through the driver wrapper layer

[stevew817]

Description

This PR is (mostly) a refactoring of existing code, in order to dispatch psa_hash_xxx calls through the driver wrapper layer. The hashing implementation on top of Mbed TLS code that used to reside inside of psa_crypto.c has been moved out into its own compile unit psa_crypto_hash.c as defined by the documentation work from @ronald-cron-arm in #3933

There is one other change: the implementation of psa_hash_compare now first calls the all-in-one psa_hash_compute function to calculate the hash into a local buffer, and then does a time-constant memcmp. This way of setting up the call tree will lead to more efficient execution when plugging in actual hardware drivers, since you'll only hit the driver once instead of potentially up to three times (setup/update/verify).

Supersedes #4043

Status

READY

Requires Backporting

NO
(new functionality)

Migrations

NO
(not present in earlier releases)

Todos

• Tests
• Documentation

Steps to test or reproduce

Test coverage through existing test cases, since the previous implementation has been fully moved inside of a 'software driver'.

[ronald-cron-arm]

CI is failing.
Error in all_sh-freebsd-test_clang_opt:

---

• test_clang_opt: build/test: clang -O0, full config

• Thu Feb 18 17:53:26 UTC 2021

---

CC aes.c

CC aesni.c

CC arc4.c

CC aria.c

CC asn1parse.c

CC asn1write.c

CC base64.c

CC bignum.c

CC blowfish.c

CC camellia.c

CC ccm.c

CC chacha20.c

CC chachapoly.c

CC cipher.c

In file included from cipher.c:29:

In file included from ../include/mbedtls/cipher_internal.h:36:

In file included from ../include/psa/crypto.h:3763:

../include/psa/crypto_struct.h:98:43: error: missing field 'ctx' initializer [-Werror,-Wmissing-field-initializers]

const struct psa_hash_operation_s v = PSA_HASH_OPERATION_INIT;

                                      ^

../include/psa/crypto_struct.h:95:36: note: expanded from macro 'PSA_HASH_OPERATION_INIT'

#define PSA_HASH_OPERATION_INIT {{0}}

                               ^

1 error generated.

make[1]: *** [Makefile:270: cipher.o] Error 1

make: *** [Makefile:17: lib] Error 2

^^^^test_clang_opt: build/test: clang -O0, full config: command make CC=clang CFLAGS=-O0 -std=c99 -pedantic -Wall -Wextra -Werror -> 2^^^^

(skipped because the build failed)

[ronald-cron-arm]

Other jobs are failing but it seems it is always the same error as above.

[stevew817]

@ronald-cron-arm should be fixed now. Strange that the local test runs are not complaining about that.

[mstarzyk-mobica]

This PR is changing the API. Please add a changelog entry.

[stevew817]

@mstarzyk-mobica It is not changing any public API. The content of the struct is checked by the CI ABI checker, but changing these hasn't required a changelog entry before...

[gilles-peskine-arm]

I confirm that psa/crypto_struct.h is a private header, so changing it is not an API change, only an ABI change. No changelog entry is necessary just because psa/crypto_struct.h changes. I don't think this PR should have a changelog entry since the feature it's adding (driver dispatch) is not in its final intended form, but requires modifying a library source file (library/psa_crypto_driver_wrappers.c).

[mstarzyk-mobica]

@mstarzyk-mobica It is not changing any public API. The content of the struct is checked by the CI ABI checker, but changing these hasn't required a changelog entry before...

Ah, alright! :)

[mstarzyk-mobica]

Looks good

[ronald-cron-arm]

Please find my comments below. I see four principal issues with the current version:

1. The hash operation context, see comment in crypto_struct.h
2. The addition of checks that pointers are not NULL where I think the Mbed TLS avoid on-purpose to do them (see the Parameter conventions section of the PSA spec for the justification of this).
3. Some code in the wrapper and in psa_crypto_hash.c would be better in psa_crypto.c in my opinion. For code being in psa_crypto.c and not in the wrapper whenever possible the rationale to me is for the wrapper code to do as little as possible to ease its generation.
4. The absence of transparent test driver hash entry points (arguably this could be done in a follow-up PR). As for the operations for which we currently support driver delegation (import/export/generate/sign_hash/verify_hash/cipher) we need support for hash operations in the transparent test driver. Due to the code reorganization of this PR the transparent test driver hash functions should just call mbedtls_transparent_test_driver_hash_xyz() functions from psa_crypto_hash.c functions. Please have a look in psa_crypto_rsa/ecp.c to see how mbedtls_transparent_test_driver_hash_xyz() functions should relate to mbedtls_psa_hash_xyz() functions and in particular the (BEYOND THIS POINT ... sections). This is a trick we have to currently do some testing of the PSA configuration with test accelerators (see the test_psa_crypto_config_basic component in all.sh) while we don't support double compilation (one compilation for the library and one compilation for the test drivers) in the tests.

[stevew817]

@ronald-cron-arm I believe I have addressed all of your feedback now.

• Dynamic allocation of driver contexts is gone, and a new paradigm for declaring driver context structures has been set up
• Test driver hooks are added for hash operations
• Superfluous operation pointer null-checks have been removed
• Input argument checking has been moved back into psa_crypto.c where it made sense (i.e. all of the ones you pointed at minus the output buffer checking, which I believe belongs with the code that is doing the actual output).

[ronald-cron-arm]

A few jobs are failing. All the ones I have checked have the same error:

psa_crypto_hash.c: In function 'mbedtls_psa_hash_update':
psa_crypto_hash.c:215:12: error: unused parameter 'input_length' [-Werror=unused-parameter]
size_t input_length )

[stevew817]

Thanks, should be fixed now

[ronald-cron-arm]

Thanks for the changes. I am happy with the way things are organized now. I find psa_crypto.c and psa_crypto_driver_wrappers.c code is simple and easy to follow. There is still a bit of pre-compiler guards tweaks to do in psa_crypto_hash.c due to the way we currently test PSA config and driver accelerators. I've tried to explain what is missing and you have psa_crypto_rsa/ecp.c/h as examples but feel free to ask if you have questions. Then, the last thing will be to update the all.sh test_psa_crypto_config_basic and test_psa_crypto_drivers components with full hash acceleration (see 17b3afc as an example).

[gilles-peskine-arm]

There's a fundamental problem with the way headers are structured. All declarations and definitions needed to compile a program using the library must be in include/**/*.h.

[gilles-peskine-arm]

To use Mbed TLS, it is sufficient to have the headers in include and compiled libraries (e.g. libmbed*.a or libmbed*.so libmbed*.dll). In particular, headers in include may not depend on headers anywhere else in the source tree.

[stevew817]

Do you have a suggestion for how to organise this, then? Or are you suggesting that psa_crypto_driver_wrappers_contexts.h move into include/psa, and that all (supposedly internal) software drivers also place their headers there?

[gilles-peskine-arm]

psa_crypto_driver_wrappers_contexts needs to move to include/psa, yes. But it also needs to lose some things, including references to the test drivers. And yes, drivers need to place their context information under include. The driver wrapper generator will produce both include/psa/something.h and library/something.c as outputs.

[stevew817]

Test driver contexts are driver contexts, too. So how would psa_crypto_driver_wrappers_contexts move to include/psa and simultaneously drop the test driver references?

[stevew817]

Not doing dynamic allocation means all driver contexts need to be fully-typed by the time the driver context union in the struct gets evaluated. So all driver contexts (including the test driver's) need to be reachable from (and thus according to your stated goal, located inside of) the library's /include folder.

So... Are we back to creating a /include/psa/drivers/ folder where all drivers (including both mbedTLS software-based and test) drivers need to put their context structure declarations? And the autogenerated include/psa/psa_crypto_driver_wrappers_contexts.h will pull in all declared drivers' context structure headers?

[gilles-peskine-arm]

Namespacing drivers as psa/drivers/*.h seems sensible to me. Mbed TLS itself wouldn't include a include/psa/drivers directory though, since we'd have nothing to put there. (Unless we put headers for the built-in implementations there?) If you package Mbed TLS with your own drivers, is it up to Mbed TLS to tell you where to put your headers?

And the autogenerated include/psa/psa_crypto_driver_wrappers_contexts.h will pull in all declared drivers' context structure headers?

Yes.

[stevew817]

Unless we put headers for the built-in implementations there

That's exactly what I was saying. They'd need to go there, since apparently all of their structures need to be declared in /include, and just dumping them flat in include/psa will (IMO) be terribly messy.

[ronald-cron-arm]

CI is OK no genuine failure.

[stevew817]

Does this fit the bill now, @gilles-peskine-arm ?

[stevew817]

All of @ronald-cron-arm 's feedback should now be addressed. I rebased as per the rewrite request, and added the other changes as new commits.

[ronald-cron-arm]

Not sure what you mean by "not just the dummy int".

[stevew817]

When a user initialises the context structure with PSA_HASH_OPERATION_INIT, the macro is built in such a way that the underlying union psa_driver_hash_context_t can be initialised with a single item, to avoid the init macro from having to know the details of the other union member's content.

But this also means the only thing that's guaranteed to be set to zero in that union is the unsigned dummy member of the union, and not the entire struct, therefore the need for a full memset (to guarantee that the driver code gets a fully-zeroized context structure upon a setup call).

[stevew817]

Updated the language to be clearer.

[ronald-cron-arm]

Thanks for the changes. The CI is ok: it is expected for ABI-API-checking to fail and the other failure is an infrastructure glitch. Almost good to me, I would just prefer to simplify the guarding of the test driver hash entry points.

[stevew817]

@ronald-cron-arm I updated the define guards like you asked.

[ronald-cron-arm]

Thanks for all your work on this. LGTM.

[ronald-cron-arm]

@tom-daubney-arm Please have a look to the last version.

[tom-daubney-arm]

Have followed through all the new changes and now all LGTM. Thanks.

[ronald-cron-arm]

@mpg @gilles-peskine-arm @yanesca Could you have the final look to this PR and merge it if this is ok ?

[gilles-peskine-arm]

I have done a design review (basically, I read the header files and looked at what functions are being created and moved around, without checking all details). Consider this approved for design.

If this was a full review with no urgency, I'd leave some minor change requests. Because none of these changes are actual bugs or raise commitments to backward compatibility, I'm going to go ahead and merge this pull request, but I've filed an issue for doing these minor changes: #4242.