[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. #23335

ballifatih · 2024-05-27T15:30:54Z

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 driver improvements:
- Remove key endianness swapping and use freshly introduced swap_key flag instead (from [hmac,regs/rtl] Add key_swap field and expose Idle status to SW #23242).
- Remove redundant HMAC-driver-specific hmac_digest_t, hmac_key_t types and use generic *word32_buf_t structs instead.
- Add placeholder oneshot call so that we can improve these later. This will later be replaced with a truly one-shat call implemented in the driver, that does not incur multiple pause/store/restore costs per block. At this point, it is there to have cleaner otcrypto_hash and otcrypto_hmac functions at cryptolib level.
Added a function to keyblob.h/c that 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.
Connect otcrypto_hash_* and otcrypto_hmac_* calls to HMAC driver. The one-shot and streaming calls are simply wrappers to HMAC calls with similar names + few error checks.
- I could use careful review on how I convert otcrypto_hash_context_t, otcrypto_hmac_context_t to driver-level hmac_ctx_t. Besides these structs, I implemented hmac_ctx_save and hmac_ctx_restore calls in hmac.c and hash.c. Unlike before, I decided to use the same context for all operations. This part is open to suggestions.
Testing support PR that includes python scripts to generate arbitrary-size HMAC vectors, as well as hardcoded hjson file generated from. We can drop this commit if needed, but I would keep it around for the sake of easier development, and follow-up bug fixing.

sw/device/lib/crypto/impl/keyblob.h

sw/device/lib/crypto/impl/mac.c

ballifatih · 2024-05-30T23:37:06Z

With the last push, //sw/device/tests/crypto/cryptotest:hmac_kat_fpga_cw340_test_rom is passing (with some local bazel changes for replacing cw310 with cw340).

jadephilipoom · 2024-05-31T15:53:45Z

sw/device/lib/crypto/drivers/hmac.c

-      // The unit for `ctx->digest_len` is bytes.
-      ctx->digest_len = kHmacSha256DigestBytes;
+      // The unit for `ctx->digest_len` is words.
+      ctx->digest_len = kHmacSha256DigestWords;


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.

jadephilipoom · 2024-05-31T15:56:27Z

sw/device/lib/crypto/drivers/hmac.h

+  /* 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),


nit: these two are missing bits

jadephilipoom · 2024-05-31T16:03:57Z

sw/device/lib/crypto/drivers/hmac.h

-                   const hmac_key_t *key);
+                   otcrypto_const_word32_buf_t *key);


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.

Sounds fine, reverting to (uint32_t*, size_t) pairs for both key and the digest.

sw/device/lib/crypto/drivers/hmac.h

jadephilipoom · 2024-06-03T09:33:21Z

sw/device/lib/crypto/impl/mac.c

+    };
+    otcrypto_const_byte_buf_t msg_buf = {
+        .len = key->config.key_length,
+        .data = (uint8_t *)unmasked_key,


nit: technically this violates strict aliasing, but unsigned char * is legal and equivalent:

Suggested change

.data = (uint8_t *)unmasked_key,

.data = (unsigned char *)unmasked_key,

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

jadephilipoom · 2024-06-03T09:49:11Z

sw/device/lib/crypto/impl/hash.c

-  // 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.


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.

Added the same comment to all four functions (as there are duplicates of the same functions in mac.c and hash.c).

sw/device/lib/crypto/impl/mac.c

sw/device/tests/crypto/hmac_functest.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]>

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 requested a review from a team as a code owner May 27, 2024 15:30

ballifatih requested review from alees24 and removed request for a team and alees24 May 27, 2024 15:30

ballifatih force-pushed the hmac-driver-cryptolib branch 3 times, most recently from 8e4b002 to ab39e92 Compare May 27, 2024 22:09

ballifatih changed the title ~~[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 calls HMAC driver.~~ [HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. May 27, 2024

ballifatih requested review from moidx, jadephilipoom and wettermo May 28, 2024 10:23

moidx reviewed May 30, 2024

View reviewed changes

sw/device/lib/crypto/impl/keyblob.h Outdated Show resolved Hide resolved

sw/device/lib/crypto/impl/mac.c Outdated Show resolved Hide resolved

sw/device/lib/crypto/impl/mac.c Show resolved Hide resolved

ballifatih force-pushed the hmac-driver-cryptolib branch 3 times, most recently from cb16622 to 9c36d42 Compare May 30, 2024 23:14

ballifatih force-pushed the hmac-driver-cryptolib branch 4 times, most recently from 4a81662 to 4d9f9c9 Compare May 31, 2024 16:10

ballifatih requested a review from a team as a code owner May 31, 2024 16:10

ballifatih requested review from hcallahan-lowrisc and removed request for a team and hcallahan-lowrisc May 31, 2024 16:10

ballifatih force-pushed the hmac-driver-cryptolib branch from 4d9f9c9 to fbbbe63 Compare May 31, 2024 18:14

jadephilipoom approved these changes Jun 3, 2024

View reviewed changes

ballifatih added 4 commits June 3, 2024 16:21

[cryptolib, mac] Connect HMAC driver to cryptolib

4c29658

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]>

ballifatih force-pushed the hmac-driver-cryptolib branch from fbbbe63 to 70f9077 Compare June 3, 2024 14:23

ballifatih added 4 commits June 3, 2024 16:40

[cryptolib] Fix hash references in other functests

6ddd5c3

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]>

[top_earlgrey, dv] Add HMAC multistream top-level test

41b7ef2

Signed-off-by: Fatih Balli <[email protected]>

ballifatih force-pushed the hmac-driver-cryptolib branch from 70f9077 to 41b7ef2 Compare June 3, 2024 14:46

moidx approved these changes Jun 3, 2024

View reviewed changes

moidx merged commit 6888004 into lowRISC:master Jun 3, 2024
31 checks passed

RyanTorok mentioned this pull request Jun 4, 2024

[cryptotest] Add test coverage of HMAC streaming API #23493

Open

ballifatih mentioned this pull request Jun 5, 2024

[hmac,difs] Update DIFs and DIF-based TLTs #22332

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. #23335

[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. #23335

ballifatih commented May 27, 2024 •

edited

Loading

ballifatih commented May 30, 2024

jadephilipoom May 31, 2024

jadephilipoom May 31, 2024

jadephilipoom May 31, 2024

ballifatih Jun 3, 2024

jadephilipoom Jun 3, 2024

ballifatih Jun 3, 2024

jadephilipoom Jun 3, 2024

ballifatih Jun 3, 2024

	.data = (uint8_t *)unmasked_key,
	.data = (unsigned char *)unmasked_key,

[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. #23335

[HMAC, cryptolib] Connect all SHA-256/384/512 and HMAC-256/384/512 to HMAC driver. #23335

Conversation

ballifatih commented May 27, 2024 • edited Loading

ballifatih commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ballifatih commented May 27, 2024 •

edited

Loading