Missing include psa_crypto_service_integration.h in psa_crypto_mac.c

[Summer-ARM]

Summary

In psa_crypto_mac.c file, it calls some psa crypto spec APIs, like 'psa_hash_setup', 'psa_hash_update'.
It will cause some errors when integration mbedtls with other project if not include 'psa_crypto_service_integration.h' header.
I think it is some for other psa_crypto_xxx.c files if it calls standards psa crypto apis.

System information

Mbed TLS version (number or commit id): development branch
Operating system and version:
Configuration (if not default, please attach config.h):
Compiler and options (if you used a pre-built binary, please indicate how you obtained it):
Additional environment information:

Expected behavior

Actual behavior

Steps to reproduce

Additional information

[gilles-peskine-arm]

Patch available: #4650 . Since we don't have a check for this in our CI, can you please let us know whether this patch works?

[Summer-ARM]

Patch available: #4650 . Since we don't have a check for this in our CI, can you please let us know whether this patch works?

Hi Gilles,
I verified the fix patch together with trusted firmware m, there are some building warnings and errors. The reason is that, in crypto_spe.h, we transform the mbedtls psa API name with prefix 'mbedcrypto__', you can check here
If you include psa_crypto_service_integration.h in header psa_crypto_core.h, the file which include psa_crypto_core.h but do not define standard psa crypto API will give warnings.
I think there needs some alignment.

[gilles-peskine-arm]

I'm sorry, I don't understand the problem. Which files give warnings now?

What about including psa_crypto_spe.h via psa/crypto_platform.h (https://github.com/gilles-peskine-arm/mbedtls/tree/psa_crypto_spm-from_platform_h), which goes well with the intended use of crypto_platform.h? I've forgotten if there was a good reason not to do this, and in any case the way crypto_spe.h is included dates back to a different project (MbedOS-PSA) and might not apply anymore.

[Summer-ARM]

I tried some tests and find that

• #include "psa_crypto_service_integration.h" must be in front of #include "psa/crypto.h"

If the order is not, then it will have similar building warning and errors, for example:
/tf-m/cmake_build/lib/ext/mbedcrypto-src/library/psa_crypto.c:33:
/tf-m/secure_fw/partitions/crypto/crypto_spe.h:23:30: warning: no previous prototype for 'mbedcrypto__psa_destroy_key' [-Wmissing-prototypes]
#define PSA_FUNCTION_NAME(x) mbedcrypto__ ## x
^~~~~~~~~~~~
/tf-m/secure_fw/partitions/crypto/crypto_spe.h:60:9: note: in expansion of macro 'PSA_FUNCTION_NAME'
PSA_FUNCTION_NAME(psa_destroy_key)
^~~~~~~~~~~~~~~~~
/tf-m/cmake_build/lib/ext/mbedcrypto-src/library/psa_crypto.c:1039:14: note: in expansion of macro 'psa_destroy_key'
psa_status_t psa_destroy_key( mbedtls_svc_key_id_t key )

The patch's idea is okay, but need to put #include "psa_crypto_service_integration.h" in front of #include "psa/crypto.h"

[gilles-peskine-arm]

#include "psa_crypto_service_integration.h" must be in front of #include "psa/crypto.h"

Indeed, I got the order wrong in psa_crypto.c. Thanks.

What about putting the inclusion in crypto_platform.h though? It would ensure that we don't risk getting the order wrong, so I'd prefer this solution if it doesn't cause a problem for TF-M.

[Summer-ARM]

The order is also wrong in psa_crypto_slot_management.c file.
I just test and find the root cause, but actually I don't know why : ) Could you share with me the reason about the order or any dependencies for the order?

I tired add '#include "psa_crypto_service_integration.h" ' in crypto_platform.h, it tests okay. But you mentioned you've forgotten if there was a good reason not to do this. So I am not sure whether it is good from design view.

[gilles-peskine-arm]

The general idea is: there's a psa_import_key (for example) both in the normal world and in the secure world. The one in the normal world is a function provided by TFM that makes RPC calls to the secure world, while the one in the secure world is the one provided by Mbed TLS. However, due to limitations of how TFM is linked, the NW and SW functions can't have the same name.

So the NW function gets the normal name, and the SW function gets names with a prefix. crypto_spe.h defines a macro mbedcrypto__psa_import_key that expands to psa_import_key. This way, any subsequent use of psa_import_key in the source code (the prototype in crypto.h, the implementation in psa_crypto.c, any place that calls the function, etc.) works as if the source code contained mbedcrypto__psa_import_key.

The macro has to be defined before any occurrence of psa_import_key in the source code, so crypto_spe.h must come early in the header inclusion order.

If including crypto_spe.h from crypto_platform.h works, let's do this. It's the most logical way: that's the sort of thing crypto_platform.h was intended for. I'll update my PR to use crypto_platform.h.

[Summer-ARM]

Thanks for your detailed explanation : ) I got it！
Feel free to close this ticket when the patch is merged.

[gilles-peskine-arm]

The pull request is up: #4673. I've put in a non-regression test, which means this shouldn't break until crypto_spe.h needs updating but we forget to do it.