[cryptolib, hmac] Update HMAC driver #23196
Conversation
ballifatih
requested review from
engdoreis,
moidx and
jadephilipoom
and removed request for
a team
| cfg_reg = bitfield_bit32_write(cfg_reg, HMAC_CFG_SHA_EN_BIT, false); | ||
| abs_mmio_write32(kHmacBaseAddr + HMAC_CFG_REG_OFFSET, cfg_reg); | ||
|
|
||
| // TODO(#23191): Use a random value from EDN to wipe. |
There was a problem hiding this comment.
Yes, it would be good to grab a value from RV_CORE_IBEX_RND_STATUS_REG_OFFSET or from entropy cached by software. I think it is ok to leave this for a future PR.
There was a problem hiding this comment.
Updated #23191 to refer to this particular RV register.
sw/device/lib/crypto/drivers/hmac.c
Outdated
| * | ||
| * @param ctx Context from which values are written to CSRs. | ||
| */ | ||
| static void restore_context(hmac_ctx_t *ctx) { |
There was a problem hiding this comment.
The ROM and cryptolib have been trying to maintain a naming convention where the verb is used as the function suffix. For example:
context_save()
context_restore()
msf_fifo_write()
etc| ctx->cfg_reg = | ||
| bitfield_bit32_write(ctx->cfg_reg, HMAC_CFG_DIGEST_SWAP_BIT, true); | ||
| // Message should be little-endian to match Ibex's endianness. | ||
| ctx->cfg_reg = | ||
| bitfield_bit32_write(ctx->cfg_reg, HMAC_CFG_ENDIAN_SWAP_BIT, false); | ||
|
|
||
| for (; len >= sizeof(uint32_t); len -= sizeof(uint32_t)) { | ||
| uint32_t data_aligned = read_32(data); | ||
| abs_mmio_write32(TOP_EARLGREY_HMAC_BASE_ADDR + HMAC_MSG_FIFO_REG_OFFSET, | ||
| data_aligned); | ||
| data += sizeof(uint32_t); | ||
| // We need to keep `sha_en` low until context is restored, see #23014. | ||
| ctx->cfg_reg = bitfield_bit32_write(ctx->cfg_reg, HMAC_CFG_SHA_EN_BIT, false); | ||
|
|
||
| // Default value for `hmac_en` is false, HMAC calls set it to true below. | ||
| ctx->cfg_reg = | ||
| bitfield_bit32_write(ctx->cfg_reg, HMAC_CFG_HMAC_EN_BIT, false); |
There was a problem hiding this comment.
Are you able to run this test (FPGA or silicon)? //SW/device/tests/crypto/cryptotest:hmac_kat
There was a problem hiding this comment.
I do not have FPGA or silicon at the moment, so I am running custom tests on sim_verilator and on sim_dv:
29b6a51
I actually managed to get my hands on CW340, but I didn't have time to setup hyperdebug. I assume this test is not part of CI then?
sw/device/lib/crypto/drivers/hmac.c
Outdated
| bitfield_bit32_write(ctx->cfg_reg, HMAC_CFG_HMAC_EN_BIT, true); | ||
| // Key values supported natively by HW are {128, 256, 384, 512, 1024}. | ||
| switch (key->len) { | ||
| case 128 / 8: |
There was a problem hiding this comment.
Do you have any enum definitions in the hmac.h file you can use here?
There was a problem hiding this comment.
We do not have anything coming from the autogenerated header but I will add the following to hmac.h, so that we do not have these naked values:
enum {
/* 128 bit key. */
kHmacKey128Bytes = 128 / 8,
/* 256 bit key. */
kHmacKey256Bytes = 256 / 8,
/* 384 bit key. */
kHmacKey384Bytes = 384 / 8,
/* 512 bit key. */
kHmacKey512Bytes = 512 / 8,
/* 1024 bit key. */
kHmacKey1024Bytes = 1024 / 8,
};
| uint32_t cfg_reg; | ||
| // Need to keep some extra info around to reconfigure HW every time. | ||
| uint32_t key[kHmacMaxKeyWords]; | ||
| // Length of `key` in words. |
There was a problem hiding this comment.
nit: Clarify that key_len == 0 to denote SHA2 mode of operation. You may want to consider adding a multi bit flag to denote HMAC enabled to simplify hardening.
hardened_bool_t hmac_mode_en;There was a problem hiding this comment.
In the hardening direction, I think there could be couple of improvements related to hmac_ctx_t. I think it would make sense to have 32b encoded field to denote which hmac_mode is running and use it to also recalculate values such as msg_block_len and digest_len. There is also another non-hardened flag hw_started.
For the moment, I would like to keep things simple so that we can focus on debugging RTL/driver issues. I have added this to the TODO items: #23191. I updated the comment line to emphasize that.
ballifatih
force-pushed
the
hmac-driver-impl
branch
from
c67893a to
34a2320
Compare
Adds streaming functions to HMAC driver that supports SHA/HMAC-256/384/512. Signed-off-by: Fatih Balli <[email protected]>
Because of the large change in HMAC driver, ecdsa_p256_verify_functest is affected. This commit fixes the HMAC calls. Signed-off-by: Fatih Balli <[email protected]>
ballifatih
force-pushed
the
hmac-driver-impl
branch
from
34a2320 to
665425c
Compare
Signed-off-by: Fatih Balli <[email protected]>
Signed-off-by: Fatih Balli <[email protected]>
ballifatih
force-pushed
the
hmac-driver-impl
branch
from
665425c to
cdd0591
Compare
|
It would be great if we can merge this soon, so that I can make a PR for cryptolib-level changes (namely, diverting @moidx I have not run cc: @wettermo as he is working on host-side test for the driver #22936. |
|
As I am actively working on cryptolib changes for HMAC, post-merge reviews are also welcome! I can adress them in the upcoming PR. |
As outline in #22731, the plan is to move SHA-2/HMAC implementations to HMAC HWIP. This PR only brings the "bottom" part of the implementation (which is the HMAC driver), and I plan to make another PR for the "top" part (which is the code that connects HMAC driver to cryptolib API).
Therefore this "bottom" part includes addition of streaming calls to the HMAC driver. I have tested the driver for all 6 functionality, but this PR only connects SHA-256 function to cryptolib, because I want to keep "top" part simple in this PR.
There is also tracker for planned changes in #23191.