[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. #23335
Conversation
ballifatih
requested review from
alees24
and removed request for
a team and
alees24
ballifatih
force-pushed
the
hmac-driver-cryptolib
branch
3 times, most recently
from
8e4b002 to
ab39e92
Compare
ballifatih
changed the title
ballifatih
force-pushed
the
hmac-driver-cryptolib
branch
3 times, most recently
from
cb16622 to
9c36d42
Compare
|
With the last push, |
ballifatih
force-pushed
the
hmac-driver-cryptolib
branch
4 times, most recently
from
4a81662 to
4d9f9c9
Compare
ballifatih
requested review from
hcallahan-lowrisc
and removed request for
a team and
hcallahan-lowrisc
ballifatih
force-pushed
the
hmac-driver-cryptolib
branch
from
4d9f9c9 to
fbbbe63
Compare
sw/device/lib/crypto/drivers/hmac.c
Outdated
| // The unit for `ctx->digest_len` is bytes. | ||
| ctx->digest_len = kHmacSha256DigestBytes; | ||
| // The unit for `ctx->digest_len` is words. | ||
| ctx->digest_len = kHmacSha256DigestWords; |
There was a problem hiding this comment.
nit: now that we have msg_block_len in bytes and digest_len in words, perhaps we could rename them to make the units clearer? e.g. msg_block_bytelen and digest_wordlen? Same for the key.
sw/device/lib/crypto/drivers/hmac.h
Outdated
| /* Internal block size for SHA-256/HMAC-256 in bits, bytes and word | ||
| respectively. */ | ||
| kHmacSha256BlockBytes = 2 * kHmacSha256DigestBytes, | ||
| kHmacSha256BlockWords = kHmacSha256BlockBytes / sizeof(uint32_t), | ||
| /* Internal block size for SHA-384/512 and HMAC-384/512 in bits, bytes and | ||
| word respectively. */ | ||
| kHmacSha512BlockBytes = 2 * kHmacSha512DigestBytes, | ||
| kHmacSha512BlockWords = kHmacSha512BlockBytes / sizeof(uint32_t), |
There was a problem hiding this comment.
nit: these two are missing bits
sw/device/lib/crypto/drivers/hmac.h
Outdated
| const hmac_key_t *key); | ||
| otcrypto_const_word32_buf_t *key); |
There was a problem hiding this comment.
So far, I've not been using top-level API types like otcrypto_const_word32_buf_t in lower levels of the cryptolib, especially the drivers. This creates a dependency between the top-level API and the driver code. It doesn't technically become circular, because the datatypes are separated out from the rest of the API, but it does mean that you need to understand top-level API types in order to understand cryptolib drivers, and if you change the top level then you have to change the driver too. Put another way, I think of otcrypto_const_word32_buf_t as "a buffer allocated by a caller outside the cryptolib" and so it feels a bit strange to me to see it in a driver, which is below the level that directly concerns the caller. I could see us deciding to just use the top-level types in drivers, but then I'd want to change all the drivers at once for consistency.
I'd prefer reinstating the hmac_key_t type or just using uint32_t * and size_t for the length.
There was a problem hiding this comment.
Sounds fine, reverting to (uint32_t*, size_t) pairs for both key and the digest.
sw/device/lib/crypto/impl/mac.c
Outdated
| }; | ||
| otcrypto_const_byte_buf_t msg_buf = { | ||
| .len = key->config.key_length, | ||
| .data = (uint8_t *)unmasked_key, |
There was a problem hiding this comment.
nit: technically this violates strict aliasing, but unsigned char * is legal and equivalent:
| .data = (uint8_t *)unmasked_key, | |
| .data = (unsigned char *)unmasked_key, |
There was a problem hiding this comment.
My understanding on the strict aliasing rules is limited, so I will go with your suggestion here. Thanks.
sw/device/lib/crypto/impl/hash.c
Outdated
| // As per the `hardened_memcpy()` documentation, it is OK to cast to | ||
| // `uint32_t *` here as long as `state` is word-aligned, which it must be | ||
| // because all its fields are. |
There was a problem hiding this comment.
I'd leave this comment in; that cast is normally unsafe, so it's good to record the justification for why it's OK here. Same for the comment in save above.
There was a problem hiding this comment.
Added the same comment to all four functions (as there are duplicates of the same functions in mac.c and hash.c).
Follow-up improvements on HMAC driver: * Remove duplicate HMAC-specific structs and use generic `*word32_buf_t` structs instead. * Add placeholder oneshot HMAC call, which will be later replaced with more efficient oneshot implementation that does not suspend HMAC during update calls. * Use `key_swap` option and avoid swapping key endianness in SW. Signed-off-by: Fatih Balli <[email protected]>
Cryptolib API uses blinded key struct to represent keys, however unmasking those keys are necessary when the underlying driver/HW stack does not support masking. In these cases we need to unmask the key. This commit defines a function for this purpose. Signed-off-by: Fatih Balli <[email protected]>
Previously, HMAC would use OTBN for 256/384/512-bit algorithms. This commit replaces those calls with that of HMAC driver. See lowRISC#22731 for more context. Signed-off-by: Fatih Balli <[email protected]>
Previously, only SHA-256 would run on HMAC HWIP. 384 and 512-bit variants were connected to OTBN. With this commit, cryptolib is updated such that calls are redirected to HMAC driver for all digest sizes. See lowRISC#22731 for more context. Signed-off-by: Fatih Balli <[email protected]>
ballifatih
force-pushed
the
hmac-driver-cryptolib
branch
from
fbbbe63 to
70f9077
Compare
This PR adds a big testing harness for HMAC. Namely, it brings: * A py script to generate a HMAC/SHA2 vector for varying sizes (using Crypto.Hash lib) * Another py script to determine random parameters and run the prev script for to obtain 20 vecs * tpl/set scripts to conver generated hjson headers to C headers * hmac_functest to drive all variants of SHA2/HMAC functions from the generated vectors. Signed-off-by: Fatih Balli <[email protected]>
Previously, RSA and ECDSA tests were using driver-level HMAC calls for SHA-256. This commit replaces it with `otcrypto_hash` calls to make it more stable, API-wise. Signed-off-by: Fatih Balli <[email protected]>
Previously, digest_len parameters were defined in individual tests. These are now exposed through hash.h. OTBN-based sha2 references/headers are also removed, as necessary parameters are now exposed through hash.h. Signed-off-by: Fatih Balli <[email protected]>
Signed-off-by: Fatih Balli <[email protected]>
ballifatih
force-pushed
the
hmac-driver-cryptolib
branch
from
70f9077 to
41b7ef2
Compare
This PR connects previously merged HMAC driver to cryptolib API. It means that all SHA-2/HMAC variants are routed to HMAC HWIP and they are not connected to the OTBN-based implementation. See #22731 for more context.
This PR includes multiple commits, where I arranged the commits with the hope to reduce reviewing effort:
hmac_digest_t,hmac_key_ttypes and use generic*word32_buf_tstructs instead.otcrypto_hashandotcrypto_hmacfunctions at cryptolib level.keyblob.h/cthat unmasks a given key. I am open to suggestions here, but I think it makes sense to have it because we have blinded->unblinded downgrading, when the underlying implementation does not support masking scheme.otcrypto_hash_*andotcrypto_hmac_*calls to HMAC driver. The one-shot and streaming calls are simply wrappers to HMAC calls with similar names + few error checks.otcrypto_hash_context_t,otcrypto_hmac_context_tto driver-levelhmac_ctx_t. Besides these structs, I implementedhmac_ctx_saveandhmac_ctx_restorecalls inhmac.candhash.c. Unlike before, I decided to use the same context for all operations. This part is open to suggestions.