lib: uuid: replace legacy crypto support with PSA API #97340
Conversation
valeriosetti
force-pushed
the
uuid-remove-legacy-crypto
branch
from
ac10f8f to
8f8076d
Compare
lib/uuid/uuid.c
Outdated
| struct uuid *out) | ||
| { | ||
| uint8_t sha_result[PSA_HASH_LENGTH(PSA_ALG_SHA_1)]; | ||
| size_t sha_len = 0; |
There was a problem hiding this comment.
| size_t sha_len = 0; | |
| size_t sha_len; |
lib/uuid/uuid.c
Outdated
| return ret; | ||
| psa_hash_abort(&hash_operation); | ||
|
|
||
| return (status == PSA_SUCCESS) ? 0 : -EINVAL; |
There was a problem hiding this comment.
Return something else than EINVAL? Also have a look at (and maybe update) the documentation for this function in include/zephyr/sys/uuid.h.
| int uuid_generate_v5(const struct uuid *ns, const void *data, size_t data_size, | ||
| struct uuid *out) | ||
| { | ||
| uint8_t sha_result[PSA_HASH_LENGTH(PSA_ALG_SHA_1)]; |
There was a problem hiding this comment.
How about we directly use out->val instead of having yet another buffer?
There was a problem hiding this comment.
I tried this and it turned out that sample.subsys.uuid test started to fail due to the fact that out->val is too small in size to contain a SHA1 result. out->val is UUID_SIZE, i.e. 16 bytes while SHA-1 is 20 bytes.
This is also explained in the official documentation for UUID.
I will revert this change
valeriosetti
force-pushed
the
uuid-remove-legacy-crypto
branch
from
8f8076d to
23d05dc
Compare
valeriosetti
force-pushed
the
uuid-remove-legacy-crypto
branch
2 times, most recently
from
0427905 to
1cc7324
Compare
zephyrbot
added
the
area: Tests
|
| @@ -75,7 +75,8 @@ int uuid_generate_v4(struct uuid *out); | |||
| * @retval 0 The UUID has been correctly generated and stored in @p out | |||
| * @retval -EINVAL @p out is not acceptable | |||
| * @retval -ENOMEM Memory allocation failed | |||
| * @retval -ENOTSUP mbedTLS returned an unrecognized error | |||
| * @retval -ENOTSUP Mbed TLS returned an unrecognized error | |||
There was a problem hiding this comment.
Also, you are returning -ENOTSUP only if you receive PSA_ERROR_NOT_SUPPORTED. This is not quite aligned.
IMO we could just return a generic error code. Trying to translate between PSA Crypto and libc error codes is... not easy.
There was a problem hiding this comment.
Are you proposing to use a catch-all error? Which one would be a good candidate?
There was a problem hiding this comment.
you are returning -ENOTSUP only if you receive PSA_ERROR_NOT_SUPPORTED
According to the PSA specs PSA_ERROR_NOT_SUPPORTED is returned by psa_hash_setup is the alg is not supported and this can happen. To me the translation to -ENOTSUP is reasonable. What would you suggest instead?
lib/uuid/uuid.c
Outdated
| case PSA_SUCCESS: | ||
| return 0; | ||
| case PSA_ERROR_INSUFFICIENT_MEMORY: | ||
| case PSA_ERROR_BUFFER_TOO_SMALL: |
There was a problem hiding this comment.
This one doesn't seem accurate or even relevant to have?
There was a problem hiding this comment.
Right, the output buffer is on the stack, so I agree this can never happen. Thanks
lib/uuid/uuid.c
Outdated
| case PSA_ERROR_INVALID_ARGUMENT: | ||
| return -EINVAL; |
There was a problem hiding this comment.
We're not expecting this either? just thinking about code size
There was a problem hiding this comment.
Right, according to the documentation this can only be returned by psa_hash_init if the algorithm is invalid, but this is not possible since it's hardcoded. I will remove it, thanks
Legacy crypto support is going to be removed in the next Mbed TLS release (which will be named TF-PSA-Crypto for the crypto support) so this commit transitions UUID library from legacy crypto to PSA API. Signed-off-by: Valerio Setti <[email protected]>
valeriosetti
force-pushed
the
uuid-remove-legacy-crypto
branch
from
1cc7324 to
f90c906
Compare
6 participants
Legacy crypto support is going to be removed in the next Mbed TLS release (which will be named TF-PSA-Crypto for the crypto support) so this commit transitions UUID library from legacy crypto to PSA API.