From 90b93b4cc078e78fb9f0cfccef547fda96383189 Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Wed, 3 Sep 2025 00:52:13 +0100 Subject: [PATCH 1/4] [ot] hw/opentitan: ot_kmac: Only zero capacity for SW KMAC w/ sideload As is explained in comments in the code, the current RTL actually exposes the entirety of the internal Keccak state, including the resultant capacity that is not a valid part of the digest, meaning that this capacity should be visible through the state registers. This is the case for all operations, except for SW-initiated operations that use a sideloaded key for KMAC. In this case, HW does indeed zero the capacity as one might expect - see: https://github.com/lowRISC/opentitan/blob/a14e715d0f425bc4986be526bc52bdeb901756bf/hw/ip/kmac/rtl/kmac_app.sv#L727-L728 Signed-off-by: Alex Jones --- hw/opentitan/ot_kmac.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/hw/opentitan/ot_kmac.c b/hw/opentitan/ot_kmac.c index eee2d4dc84cb5..f69eb41f014a4 100644 --- a/hw/opentitan/ot_kmac.c +++ b/hw/opentitan/ot_kmac.c @@ -2,6 +2,7 @@ * QEMU OpenTitan KMAC device * * Copyright (c) 2023-2025 Rivos, Inc. + * Copyright (c) 2025 lowRISC contributors. * * Author(s): * Loïc Lefort @@ -28,7 +29,6 @@ * THE SOFTWARE. * * Note: This implementation is missing some features: - * - Side-loading * - Masking (current implementation does not consume entropy) */ @@ -738,6 +738,22 @@ static void ot_kmac_process(void *opaque) g_assert_not_reached(); } + /* + * "If key is sideloaded and KMAC is SW initiated, hide the capacity + * from SW by zeroing", i.e. if not doing a SW-initiated sideloaded + * operation, the entire Keccak state (including the meaningless + * capacity bytes) should be loaded. + */ + uint32_t reg_cfg = ot_shadow_reg_peek(&s->cfg); + bool sideload = FIELD_EX32(reg_cfg, CFG_SHADOWED, SIDELOAD) != 0; + if (!sideload || s->current_app) { + static_assert(sizeof(s->ltc_state.sha3.sb) == KECCAK_STATE_BYTES, + "LibTomCrypt's Keccak state is an unexpected size"); + /* manually extract entire Keccak state, including capacity */ + memcpy(&s->keccak_state[0], s->ltc_state.sha3.sb, + KECCAK_STATE_BYTES); + } + if (s->current_app) { /* App mode, send response and go back to IDLE state */ if (s->current_app->fn) { From a05dd14653e04572094e5658cc43cb16b3230fcf Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Sun, 31 Aug 2025 22:16:57 +0100 Subject: [PATCH 2/4] [ot] hw/opentitan: ot_kmac: Handle re-entrant app requests The KMAC app request API already errors if we have an existing request pending from an APP, but there is an edge case with re-entrancy. Consider: 1. A device calls a kmac->app_request(data). 2. The KMAC processes the data and responds, calling dev_app->fn(resp) 3. As a result of the response, the device wants to process more KMAC requests, so it sends another app request. 4. KMAC thinks the app is not pending but still sees the current app set, so it thinks it is receiving more data. It will try and process that data and then then transition to idle, but the data that was expected to be processed has not been processed. This re-entrancy should be allowed because we are essentially just trying to "queue" an operation now that the previous one is completed, and we want it to run after the KMAC is idle again (i.e. state reset). So, by simply modifying the existing check we can use the existing app request/pending mechanisms to allow safe re-entrancy (also without potential for stack issues). Signed-off-by: Alex Jones --- hw/opentitan/ot_kmac.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/opentitan/ot_kmac.c b/hw/opentitan/ot_kmac.c index f69eb41f014a4..474a9490298de 100644 --- a/hw/opentitan/ot_kmac.c +++ b/hw/opentitan/ot_kmac.c @@ -1707,8 +1707,9 @@ static void ot_kmac_app_request(OtKMACState *s, unsigned app_idx, app->req_pending = true; /* check if app already started */ - if (s->current_app == app) { - /* yes, trigger deferred compute */ + if (s->current_app == app && + (s->state == KMAC_ST_IDLE || s->state == KMAC_ST_MSG_FEED)) { + /* yes, receiving more data, trigger deferred compute */ qemu_bh_schedule(s->bh); } else { /* no, mark as pending and try to start */ From 8eaafedbe2d455e0f8777ffc687abde0b34c3dbe Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Wed, 3 Sep 2025 01:00:23 +0100 Subject: [PATCH 3/4] [ot] hw/opentitan: ot_kmac: Error on invalid sideloaded key for SW KMAC The RTL actually does error even in SW-initiated KMAC commands that are making use of an invalid sideloaded key, with the `app_id` that is reported in the error code just defaulting to zero. Signed-off-by: Alex Jones --- hw/opentitan/ot_kmac.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/opentitan/ot_kmac.c b/hw/opentitan/ot_kmac.c index 474a9490298de..9a31c0e67a3f7 100644 --- a/hw/opentitan/ot_kmac.c +++ b/hw/opentitan/ot_kmac.c @@ -622,10 +622,11 @@ static void ot_kmac_get_key(OtKMACState *s, uint8_t *key, size_t *keylen) for (size_t ix = 0; ix < OT_KMAC_KEY_SIZE; ix++) { key[ix] = sl_key->share0[ix] ^ sl_key->share1[ix]; } - /* only check key validity in App mode */ - if (s->current_app && !sl_key->valid) { - ot_kmac_report_error(s, OT_KMAC_ERR_KEY_NOT_VALID, - s->current_app->index); + + if (!sl_key->valid) { + /* HW defaults to info = app_id = 0 when in a SW operation. */ + uint32_t err_info = s->current_app ? s->current_app->index : 0; + ot_kmac_report_error(s, OT_KMAC_ERR_KEY_NOT_VALID, err_info); } return; } From 5d59b42ee6236654cd5a3302cd74f95ae4497780 Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Wed, 3 Sep 2025 01:02:40 +0100 Subject: [PATCH 4/4] [ot] hw/opentitan: ot_kmac: Implement SW error handling Previously, the KMAC `CMD.ERR_PROCESSED` bit was just clearing the alerts, but not actually doing anything to the internal KMAC state. This commit implements full error handling for both HW and SW operation errors. The logic is the same for both types - upon an erroneous command, the user must write the `err_processed` bit, upon which the KMAC will flush its internal state by sending a `Process` command, followed by an `Absorb` command when done. The key differences are: - For HW app interface errors: as soon as all the data is received, HW is responded to so we don't stall HW, but the FSM won't return to idle until SW acknowledges the error. If SW acknowledges the error before the HW request data is finished sending, then KMAC first waits for the full request, then responds, then returns to idle. - For SW app interface errors: the internal KMAC state is flushed as soon as the error is processed, ignoring any further message feed or commands and immediately processing, and then finishing ('Done') when absorbed, before returning to idle. Signed-off-by: Alex Jones --- hw/opentitan/ot_kmac.c | 124 +++++++++++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 24 deletions(-) diff --git a/hw/opentitan/ot_kmac.c b/hw/opentitan/ot_kmac.c index 9a31c0e67a3f7..879aabb1b8218 100644 --- a/hw/opentitan/ot_kmac.c +++ b/hw/opentitan/ot_kmac.c @@ -402,6 +402,7 @@ struct OtKMACState { OtKMACFsmState state; /* Main FSM state */ bool invalid_state_read; + bool error_awaiting_sw; /* error awaiting SW acknowledgement */ hash_state ltc_state; /* TomCrypt hash state */ uint8_t keccak_state[KECCAK_STATE_BYTES]; @@ -501,6 +502,7 @@ static void ot_kmac_report_error(OtKMACState *s, int code, uint32_t info) error = FIELD_DP32(error, ERR_CODE, CODE, code); error = FIELD_DP32(error, ERR_CODE, INFO, info); + s->error_awaiting_sw = true; s->regs[R_ERR_CODE] = error; s->regs[R_INTR_STATE] |= INTR_KMAC_ERR_MASK; ot_kmac_update_irq(s); @@ -663,6 +665,23 @@ static void ot_kmac_reset_state(OtKMACState *s) static void ot_kmac_start_pending_app(OtKMACState *s); +static void ot_kmac_return_to_idle(OtKMACState *s) +{ + /* flush state */ + ot_kmac_change_fsm_state(s, KMAC_ST_IDLE); + ot_kmac_reset_state(s); + ot_kmac_cancel_bh(s); + /* now is a good time to check for pending app requests */ + ot_kmac_start_pending_app(s); +} + +static void ot_kmac_complete_app_req(OtKMACState *s) +{ + trace_ot_kmac_app_finished(s->ot_id, s->current_app->index); + s->current_app = NULL; + ot_kmac_return_to_idle(s); +} + /* BH handler for processing FIFO and compute */ static void ot_kmac_process(void *opaque) { @@ -764,13 +783,9 @@ static void ot_kmac_process(void *opaque) memset(&rsp.digest_share1[0], 0, sizeof(rsp.digest_share1)); s->current_app->fn(s->current_app->opaque, &rsp); } - ot_kmac_change_fsm_state(s, KMAC_ST_IDLE); - ot_kmac_reset_state(s); - ot_kmac_cancel_bh(s); - trace_ot_kmac_app_finished(s->ot_id, s->current_app->index); - s->current_app = NULL; - /* now is a good time to check for pending app requests */ - ot_kmac_start_pending_app(s); + if (!s->error_awaiting_sw) { + ot_kmac_complete_app_req(s); + } } else { /* SW mode, go to ABSORBED state */ ot_kmac_change_fsm_state(s, KMAC_ST_ABSORBED); @@ -993,8 +1008,81 @@ static void ot_kmac_process_start(OtKMACState *s) } } -static void ot_kmac_process_sw_command(OtKMACState *s, int cmd) +static void ot_kmac_sw_err_processed(OtKMACState *s, int cmd) { + s->error_awaiting_sw = false; + + if (s->current_app) { + /* + * If we have already received the entire app request data and sent a + * response, now we just need SW acknowledgement to complete. + * + * If we haven't got all the data, we should wait for it all before + * sending a response (but we store SW acknowledgement). + */ + if (s->state == KMAC_ST_PROCESSING || s->state == KMAC_ST_SQUEEZING) { + ot_kmac_complete_app_req(s); + } + } else { + /* for SW: ignore further msg feed, absorb, report done & go to idle */ + switch (s->state) { + case KMAC_ST_IDLE: + case KMAC_ST_MSG_FEED: + ot_kmac_change_fsm_state(s, KMAC_ST_PROCESSING); + /* fallthrough */ + case KMAC_ST_PROCESSING: + case KMAC_ST_SQUEEZING: + ot_kmac_process((void *)s); + /* fallthrough */ + case KMAC_ST_ABSORBED: + ot_kmac_return_to_idle(s); + break; + case KMAC_ST_TERMINAL_ERROR: + break; + default: + g_assert_not_reached(); + } + } + + /* Clear the status / alert */ + s->regs[R_STATUS] &= ~R_STATUS_ALERT_RECOV_CTRL_UPDATE_ERR_MASK; + ot_kmac_update_alert(s); + + /* Clear the error */ + s->regs[R_ERR_CODE] = 0u; + + /* SW shoud not send a command along with the err_processed bit */ + if (cmd != OT_KMAC_CMD_NONE) { + if (s->current_app) { + ot_kmac_report_error(s, OT_KMAC_ERR_SW_ISSUED_CMD_IN_APP_ACTIVE, + cmd); + } else { + /* see error encoding in (hw/ip/kmac/rtl/kmac_pkg.sv) */ + uint32_t info = (1 << 11u); + info |= (uint32_t)s->state << 8u; + info |= cmd; + ot_kmac_report_error(s, OT_KMAC_ERR_SW_CMD_SEQUENCE, info); + } + } +} + + +static void ot_kmac_process_sw_command(OtKMACState *s, uint32_t cmd_reg) +{ + bool err_processed = (cmd_reg & R_CMD_ERR_PROCESSED_MASK) != 0; + int cmd = (int)FIELD_EX32(cmd_reg, CMD, CMD); + if (s->error_awaiting_sw) { + if (err_processed) { + ot_kmac_sw_err_processed(s, cmd); + } else { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: %s: Control write without err_processed whilst " + "KMAC is handling an error\n", + __func__, s->ot_id); + } + return; + } + uint32_t cfg = ot_shadow_reg_peek(&s->cfg); bool err_swsequence = false; bool err_modestrength = false; @@ -1075,12 +1163,7 @@ static void ot_kmac_process_sw_command(OtKMACState *s, int cmd) ot_kmac_change_fsm_state(s, KMAC_ST_SQUEEZING); ot_kmac_trigger_deferred_bh(s); } else if (cmd == OT_KMAC_CMD_DONE) { - /* flush state */ - ot_kmac_change_fsm_state(s, KMAC_ST_IDLE); - ot_kmac_reset_state(s); - ot_kmac_cancel_bh(s); - /* now is a good time to check for pending app requests */ - ot_kmac_start_pending_app(s); + ot_kmac_return_to_idle(s); } else { err_swsequence = true; } @@ -1124,7 +1207,7 @@ static void ot_kmac_process_sw_command(OtKMACState *s, int cmd) g_assert_not_reached(); } ot_kmac_report_error(s, code, info); - } else { + } else if (!s->error_awaiting_sw) { s->regs[R_ERR_CODE] = 0; } } @@ -1339,9 +1422,7 @@ static void ot_kmac_regs_write(void *opaque, hwaddr addr, uint64_t value, } break; case R_CMD: { - int cmd = (int)FIELD_EX32(val32, CMD, CMD); - - ot_kmac_process_sw_command(s, cmd); + ot_kmac_process_sw_command(s, val32); if (val32 & R_CMD_ENTROPY_REQ_MASK) { /* TODO: implement entropy */ @@ -1356,12 +1437,6 @@ static void ot_kmac_regs_write(void *opaque, hwaddr addr, uint64_t value, "%s: %s: CMD.HASH_CNT_CLR is not supported\n", __func__, s->ot_id); } - - if (val32 & R_CMD_ERR_PROCESSED_MASK) { - /* TODO: implement entropy */ - s->regs[R_STATUS] &= ~R_STATUS_ALERT_RECOV_CTRL_UPDATE_ERR_MASK; - ot_kmac_update_alert(s); - } break; } case R_ENTROPY_PERIOD: @@ -1777,6 +1852,7 @@ static void ot_kmac_reset_enter(Object *obj, ResetType type) s->current_app = NULL; s->pending_apps = 0; s->invalid_state_read = false; + s->error_awaiting_sw = false; memset(s->regs, 0, sizeof(*(s->regs))); s->regs[R_STATUS] = 0x4001u; ot_shadow_reg_init(&s->cfg, 0u);