Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hmac, sw] Revert hash stop hang workaround #25594

Merged
merged 2 commits into from
Dec 16, 2024
Merged

[hmac, sw] Revert hash stop hang workaround #25594

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2022cc
[hmac, sw] Revert hash stop hang workaround
andrea-caforio Dec 11, 2024
5b0c395
[hmac, sw] Reintroduce idle polling timeout
andrea-caforio Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions hw/ip/hmac/doc/programmers_guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,24 +88,17 @@ When SW doesn't know each instant at which a full message block is available, it

The context that needs to be saved and restored is in the following registers: [`CFG`](registers.md#cfg), [`DIGEST_*`](registers.md#digest), and [`MSG_LENGTH_*`](registers.md#msg_length_lower).

**Due to an RTL bug, the hardware may not be able to easily recover from a `CMD.hash_stop` command (for more details, refer to [Issue #24767](https://github.com/lowRISC/opentitan/issues/24767)).
To avoid the hardware from entering this state, a stop-gap software workaround solution should be used (see [PR #24944](https://github.com/lowRISC/opentitan/pull/24944) for details).
The example usage below includes this stop-gap workaround solution marked in italics whereas the original guidance is crossed out.**

Each new message stream needs to be started *once* by setting the `CMD.hash_start` bit and finalized *once* by setting the `CMD.hash_process` bit.
To switch from one message stream to another, ~~set the `CMD.hash_stop` bit, wait for the `hmac_done` interrupt (or status bit)~~ _at a block boundary, insert delay which should be equivalent to at least 80 clock cycles_, save one context, _trigger `CMD.hash_process` to move the FSMs into a stable state_ and restore the other context, and then set the `CMD.hash_continue` bit.
To switch from one message stream to another, set the `CMD.hash_stop` bit, wait for the `hmac_done` interrupt (or status bit), save one context and restore the other, and then set the `CMD.hash_continue` bit.

Here is an example usage pattern of this feature:
1. Start processing message stream A by configuring HMAC and then setting the `CMD.hash_start` bit.
1. Write an arbitrary number of message blocks to HMAC's `MSG_FIFO`.
1. _Insert a delay of least 80 clock cycles._
~~1. Stop HMAC by setting the `CMD.hash_stop` bit and wait for the `hmac_done` interrupt (or poll the interrupt status register).~~
1. Stop HMAC by setting the `CMD.hash_stop` bit and wait for the `hmac_done` interrupt (or poll the interrupt status register).
1. Save the context by reading the `DIGEST_0`..`15` and `MSG_LENGTH_`{`LOWER`,`UPPER`} registers.
If the operation is keyed HMAC, the values of `KEY_0`..`X` registers also need to be maintained as part of the context, where `X` is the last register used for the given key length (e.g. for HMAC-256, `X=7`).
However, key registers cannot be read from SW, therefore SW must maintain key values as part of its own context during initialization.
Similarly, the value of the `CFG` register must also be preserved, and SW should keep its value separately, instead of reading it from `CFG` register.
1. _Trigger `CMD.hash_process`._
1. _Wait for the `hmac_done` interrupt (or poll the interrupt status register)._
1. Disable `sha_en` by updating `CFG` register, in order to clear the digest from stream A.
This is necessary to also prevent leakage of intermediate context of one SW thread to another.
1. Repeat steps 1-5 for message stream B.
Expand Down
1 change: 0 additions & 1 deletion sw/device/lib/crypto/drivers/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ cc_library(
"//sw/device/lib/base:macros",
"//sw/device/lib/base:memory",
"//sw/device/lib/crypto/impl:status",
"//sw/device/lib/runtime:ibex",
],
)

Expand Down
85 changes: 13 additions & 72 deletions sw/device/lib/crypto/drivers/hmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "sw/device/lib/base/hardened.h"
#include "sw/device/lib/base/memory.h"
#include "sw/device/lib/crypto/impl/status.h"
#include "sw/device/lib/runtime/ibex.h"

#include "hmac_regs.h" // Generated.
#include "hw/top_earlgrey/sw/autogen/top_earlgrey.h"
Expand Down Expand Up @@ -68,19 +67,6 @@ OT_ASSERT_ENUM_VALUE(HMAC_DIGEST_15_REG_OFFSET, HMAC_DIGEST_14_REG_OFFSET + 4);
enum {
/* The beginning of the address space of HMAC. */
kHmacBaseAddr = TOP_EARLGREY_HMAC_BASE_ADDR,
/* Timeout value for the polling
* As min 3 clock cycles are required to perform a read access to the STATUS
* hmac_idle register. And as we should observe 240 cycles (80 for the inner
* key, 80 for the outer key, and 80 for the result of the first round), plus
* 80 for msg itself -> 360 cycles in total as max (when HMAC is enabled).
* Which means, 360/3=120 loops before having the IDLE state.
* Let's take a large margin and consider that 200 loops are enough.
*/
kNumIterTimeout = 200,
/* Temporary delay linked to issue #24767, which should be equivalent to at
* least 80 clock cycles of the HMAC clock plus a margin.
*/
kHmacTmpDelay = 200,
};

/**
Expand All @@ -89,6 +75,8 @@ enum {
* It returns error if HMAC HWIP becomes idle without firing `hmac_done`
* interrupt.
*
* TODO(#23191): It might be beneficial to have a timeout value for the polling.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a plain revert of the other commits is fine and the approach is good, it's easier to review this.

What is not ideal is that it means to re-introduce this TODO here which got resolved by the workaround. Would it be possible to do a separate commit or even PR (whatever you prefer @andrea-caforio ) to resolve this? You can take inspiration from the reverted commits on how to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @vogelpi for the review and catching this issue. I reinstantiated the polling timeout implementation in a separate commit as to not spam the PR list with code that has already been review before.

* @return Result of the operation.
*/
OT_WARN_UNUSED_RESULT
Expand All @@ -97,13 +85,8 @@ static status_t hmac_idle_wait(void) {
// Initialize `status_reg = 0` so that the loop starts with the assumption
// that HMAC HWIP is not idle.
uint32_t status_reg = 0;
uint32_t attempt_cnt = 0;
while (bitfield_bit32_read(status_reg, HMAC_STATUS_HMAC_IDLE_BIT) == 0) {
status_reg = abs_mmio_read32(kHmacBaseAddr + HMAC_STATUS_REG_OFFSET);
attempt_cnt++;
if (attempt_cnt >= kNumIterTimeout) {
return OTCRYPTO_FATAL_ERR;
}
}

// Verify that HMAC HWIP raises `hmac_done` bit.
Expand Down Expand Up @@ -273,41 +256,6 @@ static void msg_fifo_write(const uint8_t *message, size_t message_len) {
}
}

/**
* Temporary workaround linked to issue #24767
*
* The HMAC HWIP is not told to stop. This will cause the HW to be in an
* unexpected state, which could be exited by trigerring a simple HASH process
* operation. Before doing that, the context should be saved (message length
* and digest). This context is only available after a duration equivalent to
* 64 clock cycles in SHA2-256 and 80 clock cycles in SHA2-384/512, after the
* message length is on a block boundary (512 for SHA2-256 or 1024 bits for
* SHA2-384/512).
*
* @param[out] ctx Context to which values are written.
*/
static status_t tmp_avoid_hw_hang(hmac_ctx_t *ctx) {
// Insert a delay
ibex_timeout_t timeout;
timeout.cycles = kHmacTmpDelay;
timeout.start = ibex_mcycle_read();
while (!ibex_timeout_check(&timeout)) {
// NULL statement
}

// Save current context as it is updated after each block even if stop is not
// triggered
context_save(ctx);

// Trigger hash_process
uint32_t cmd_reg = abs_mmio_read32(kHmacBaseAddr + HMAC_CMD_REG_OFFSET);
cmd_reg = bitfield_bit32_write(cmd_reg, HMAC_CMD_HASH_PROCESS_BIT, true);
abs_mmio_write32(kHmacBaseAddr + HMAC_CMD_REG_OFFSET, cmd_reg);

// Wait for HMAC HWIP operation to be completed.
return hmac_idle_wait();
}

/**
* For given `hmac_mode`, derive the matching CFG value and block/digest
* lengths.
Expand Down Expand Up @@ -449,24 +397,17 @@ status_t hmac_update(hmac_ctx_t *ctx, const uint8_t *data, size_t len) {
// Keep writing incoming bytes
msg_fifo_write(data, len - leftover_len);

/*
* TODO should be uncommented once the issue #24767 will be solved in the HW
* and tmp_avoid_hw_hang should be removed.
*
* // Time to tell HMAC HWIP to stop, because we do not have enough message
* // bytes for another round.
* uint32_t cmd_reg =
* bitfield_bit32_write(HMAC_CMD_REG_RESVAL, HMAC_CMD_HASH_STOP_BIT, 1);
* abs_mmio_write32(kHmacBaseAddr + HMAC_CMD_REG_OFFSET, cmd_reg);
*
* // Wait for HMAC HWIP operation to be completed.
* HARDENED_TRY(hmac_idle_wait());
*
* // Store context into `ctx`.
* context_save(ctx);
*/

tmp_avoid_hw_hang(ctx);
// Time to tell HMAC HWIP to stop, because we do not have enough message
// bytes for another round.
uint32_t cmd_reg =
bitfield_bit32_write(HMAC_CMD_REG_RESVAL, HMAC_CMD_HASH_STOP_BIT, 1);
abs_mmio_write32(kHmacBaseAddr + HMAC_CMD_REG_OFFSET, cmd_reg);

// Wait for HMAC HWIP operation to be completed.
HARDENED_TRY(hmac_idle_wait());

// Store context into `ctx`.
context_save(ctx);

// Write leftover bytes to `partial_block`, so that future update/final call
// can feed them to HMAC HWIP.
Expand Down
Loading