[keymgr] Continued security hardening
- Add redundancy to kmac interface counter
- Add sparse encoding to kmac transfer fsm
- Make the fault / error behavior more consistent. Faults will always cause keymgr to transition into "Disabled" state if an operation is ongoing. When transitioning because of a fault, the key state is always "wiped" with entropy. Once transitioned into disable state, the behavior will always be consistent, ie, random data will be used for operations, and the resulting garbage data is also used to update the state.
Signed-off-by: Timothy Chen <timothytim@google.com>
diff --git a/hw/ip/keymgr/data/keymgr.hjson b/hw/ip/keymgr/data/keymgr.hjson
index 2c3fe72..e0c6582 100644
--- a/hw/ip/keymgr/data/keymgr.hjson
+++ b/hw/ip/keymgr/data/keymgr.hjson
@@ -396,7 +396,8 @@
resval: "0"
desc: '''
Depending on the value programmed, a different side load key slot is cleared.
- If the value programmed is not one of the enumerated values below, ALL side load key slots are continuously cleard.
+ If the value programmed is not one of the enumerated values below, ALL side
+ load key slots are continuously cleared.
''',
enum: [
{ value: "0",
@@ -930,6 +931,11 @@
resval: "0x0"
desc: "Control FSM integrity error.",
},
+ { bits: "6",
+ name: "CTRL_FSM_CNT",
+ resval: "0x0"
+ desc: "Control FSM counter integrity error.",
+ },
]
},
diff --git a/hw/ip/keymgr/doc/_index.md b/hw/ip/keymgr/doc/_index.md
index 72ca87b..deae032 100644
--- a/hw/ip/keymgr/doc/_index.md
+++ b/hw/ip/keymgr/doc/_index.md
@@ -241,7 +241,7 @@
In addition to alerts and interrupts, key manager may also update the working state key and relevant outputs based on current state.
See the tables below for an enumeration.
-| Current State | Invalid Command | Invalid Output | Invalid Input | Invalid Operation |
+| Current State | Invalid States | Invalid Output | Invalid Input | Invalid Operation |
| ------------- | ----------------| ---------------|---------------|---------------------|
| Reset | Not Possible | Not Possible | Not possible | Not updated |
| Initialized | Updated | Updated | Not updated | Not updated |
diff --git a/hw/ip/keymgr/keymgr.core b/hw/ip/keymgr/keymgr.core
index ae49a51..f934a36 100644
--- a/hw/ip/keymgr/keymgr.core
+++ b/hw/ip/keymgr/keymgr.core
@@ -22,6 +22,7 @@
- rtl/keymgr_sideload_key.sv
- rtl/keymgr_ctrl.sv
- rtl/keymgr_cfg_en.sv
+ - rtl/keymgr_cnt.sv
- rtl/keymgr_kmac_if.sv
- rtl/keymgr_input_checks.sv
- rtl/keymgr_reseed_ctrl.sv
diff --git a/hw/ip/keymgr/rtl/keymgr.sv b/hw/ip/keymgr/rtl/keymgr.sv
index 32fabe8..1a73827 100644
--- a/hw/ip/keymgr/rtl/keymgr.sv
+++ b/hw/ip/keymgr/rtl/keymgr.sv
@@ -534,12 +534,14 @@
assign hw2reg.fault_status.regfile_intg.d = fault_code[FaultRegFileIntg];
assign hw2reg.fault_status.shadow.d = fault_code[FaultShadow];
assign hw2reg.fault_status.ctrl_fsm_intg.d = fault_code[FaultCtrlFsm];
+ assign hw2reg.fault_status.ctrl_fsm_cnt.d = fault_code[FaultCtrlCnt];
assign hw2reg.fault_status.cmd.de = 1'b1;
assign hw2reg.fault_status.kmac_fsm.de = 1'b1;
assign hw2reg.fault_status.kmac_op.de = 1'b1;
assign hw2reg.fault_status.regfile_intg.de = 1'b1;
assign hw2reg.fault_status.ctrl_fsm_intg.de = 1'b1;
assign hw2reg.fault_status.shadow.de = 1'b1;
+ assign hw2reg.fault_status.ctrl_fsm_cnt.de = 1'b1;
// There are two types of alerts
// - alerts for hardware errors, these could not have been generated by software.
diff --git a/hw/ip/keymgr/rtl/keymgr_cnt.sv b/hw/ip/keymgr/rtl/keymgr_cnt.sv
new file mode 100644
index 0000000..7834eb2
--- /dev/null
+++ b/hw/ip/keymgr/rtl/keymgr_cnt.sv
@@ -0,0 +1,112 @@
+// Copyright lowRISC contributors.
+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+//
+// Key manager interface to kmac
+//
+
+module keymgr_cnt import keymgr_pkg::*; #(
+ parameter int Width = 2,
+ parameter bit OutSelDnCnt = 1, // 0 selects up count
+ parameter keymgr_cnt_style_e CntStyle = CrossCnt
+) (
+ input clk_i,
+ input rst_ni,
+ input clr_i,
+ input set_i,
+ input [Width-1:0] set_cnt_i,
+ input en_i,
+ output logic [Width-1:0] cnt_o,
+ output logic err_o
+);
+
+ localparam int CntCopies = (CntStyle == DupCnt) ? 2 : 1;
+
+ typedef enum logic [1:0] {
+ CmpInvalid = 2'b01,
+ CmpValid = 2'b10
+ } cmp_valid_e;
+
+ cmp_valid_e cmp_valid;
+ logic [CntCopies-1:0][Width-1:0] up_cnt_d, up_cnt_d_buf;
+ logic [CntCopies-1:0][Width-1:0] up_cnt_q;
+ logic [Width-1:0] max_val;
+ logic err;
+
+ always_ff @(posedge clk_i or negedge rst_ni) begin
+ if (!rst_ni) begin
+ max_val <= '{default: '1};
+ end else if (set_i && OutSelDnCnt) begin
+ max_val <= set_cnt_i;
+ end
+ end
+
+ for (genvar i = 0; i < CntCopies; i++) begin : gen_cnts
+ // up-count
+ assign up_cnt_d[i] = (clr_i) ? '0 :
+ (en_i & up_cnt_q[i] < max_val) ? up_cnt_q[i] + 1'b1 :
+ up_cnt_q[i];
+
+ prim_buf #(
+ .Width(Width)
+ ) u_buf (
+ .in_i(up_cnt_d[i]),
+ .out_o(up_cnt_d_buf[i])
+ );
+
+ prim_flop #(
+ .Width(Width),
+ .ResetValue('0)
+ ) u_cnt_flop (
+ .clk_i,
+ .rst_ni,
+ .d_i(up_cnt_d_buf[i]),
+ .q_o(up_cnt_q[i])
+ );
+ end
+
+ if (CntStyle == CrossCnt) begin : gen_cross_cnt_hardening
+ logic [Width-1:0] down_cnt;
+ logic [Width-1:0] sum;
+
+ always_ff @(posedge clk_i or negedge rst_ni) begin
+ if (!rst_ni) begin
+ cmp_valid <= CmpInvalid;
+ end else if (clr_i) begin
+ cmp_valid <= CmpInvalid;
+ end else if ((cmp_valid == CmpInvalid) && set_i) begin
+ cmp_valid <= CmpValid;
+ end
+ end
+
+ // down-count
+ always_ff @(posedge clk_i or negedge rst_ni) begin
+ if (!rst_ni) begin
+ down_cnt <= '0;
+ end else if (clr_i) begin
+ down_cnt <= '0;
+ end else if (set_i) begin
+ down_cnt <= set_cnt_i;
+ end else if (en_i && down_cnt >'0) begin
+ down_cnt <= down_cnt - 1'b1;
+ end
+ end
+
+ logic msb;
+ assign {msb, sum} = down_cnt + up_cnt_q[0];
+ assign cnt_o = OutSelDnCnt ? down_cnt : up_cnt_q[0];
+ assign err = max_val != sum | msb;
+
+ end else if (CntStyle == DupCnt) begin : gen_dup_cnt_hardening
+ // duplicate count compare is always valid
+ assign cmp_valid = CmpValid;
+ assign cnt_o = up_cnt_q[0];
+ assign err = (up_cnt_q[0] != up_cnt_q[1]);
+ end
+
+ // if the compare flag is not a valid enum, treat it like an error.
+ assign err_o = (cmp_valid == CmpValid) ? err :
+ (cmp_valid == CmpInvalid) ? '0 : 1'b1;
+
+
+endmodule // keymgr_cnt
diff --git a/hw/ip/keymgr/rtl/keymgr_ctrl.sv b/hw/ip/keymgr/rtl/keymgr_ctrl.sv
index d931b6f..3926781 100644
--- a/hw/ip/keymgr/rtl/keymgr_ctrl.sv
+++ b/hw/ip/keymgr/rtl/keymgr_ctrl.sv
@@ -183,14 +183,14 @@
assign kmac_out_valid = valid_data_chk(kmac_data_i[0]) & valid_data_chk(kmac_data_i[1]);
// error definition
- assign op_fault_err_d = |fault_o | ~kmac_out_valid;
+ assign op_fault_err_d = |fault_o | ~kmac_out_valid | op_fault_err_q;
assign op_err = kmac_input_invalid_i | invalid_op;
// key update conditions
- assign key_update = (op_ack | op_update) & (advance_sel | disable_sel);
+ assign key_update = advance_sel | disable_sel;
// external collateral update conditions
- assign data_update = op_ack & gen_sel;
+ assign data_update = gen_sel;
always_ff @(posedge clk_i or negedge rst_ni) begin
@@ -262,7 +262,6 @@
wipe_key_o = 1'b0;
key_update_vld = 1'b0;
-
// if a wipe request arrives, immediately destroy the
// keys regardless of current state
@@ -285,8 +284,8 @@
end
KeyUpdateKmac: begin
- data_valid_o = data_update & ~op_fault_err & ~op_err;
- key_update_vld = key_update & ~op_fault_err & ~op_err;
+ data_valid_o = data_update & ~op_err;
+ key_update_vld = key_update & ~op_err;
key_state_d[cdi_sel_o] = key_update_vld ? kmac_data_i : key_state_q[cdi_sel_o];
end
@@ -309,15 +308,20 @@
endcase // unique case (update_sel)
end
- always_ff @(posedge clk_i or negedge rst_ni) begin
- if (!rst_ni) begin
- cnt <= '0;
- end else if (op_ack || random_ack) begin
- cnt <= '0;
- end else if (op_update) begin
- cnt <= cnt + 1'b1;
- end
- end
+ logic cnt_err;
+ keymgr_cnt #(
+ .Width(CntWidth),
+ .CntStyle(DupCnt)
+ ) u_cnt (
+ .clk_i,
+ .rst_ni,
+ .clr_i(op_ack | random_ack),
+ .set_i('0),
+ .set_cnt_i('0),
+ .en_i(op_update),
+ .cnt_o(cnt),
+ .err_o(cnt_err)
+ );
// TODO: Create a no select option, do not leave this as binary
assign hw_sel_o = gen_out_hw_sel ? HwKey : SwKey;
@@ -555,17 +559,17 @@
endcase // unique case (state_q)
end
- // if working over multiple CDIs, a fault of any
+ // If working over multiple CDIs, a fault of any
// is considered an overall fault
- logic clr_err;
-
+ //
+ // faults are always permanently retained, and will be used
+ // to transition to control FSM back into DISABLED state
+ // if it is somehow glitched OUT of it.
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
op_fault_err_q <= '0;
- end else if (op_update || op_ack) begin
+ end else begin
op_fault_err_q <= op_fault_err_d;
- end else if (op_fault_err_q && clr_err) begin
- op_fault_err_q <= '0;
end
end
@@ -583,14 +587,8 @@
id_en_o = 1'b0;
gen_en_o = 1'b0;
- clr_err = 1'b0;
-
unique case (op_state_q)
StIdle: begin
- // errors are only cleared once the main control FSM is confirmed to be
- // be in disabled state.
- clr_err = disabled;
-
if (random_req) begin
op_state_d = StRandomize;
end else if (adv_req) begin
@@ -628,8 +626,8 @@
// Invalidate keys under the following conditions
if (op_ack || op_update) begin
- op_update_sel = op_fault_err ? KeyUpdateWipe :
- disabled ? KeyUpdateInvalid : KeyUpdateKmac;
+ op_update_sel = disabled ? KeyUpdateInvalid :
+ op_fault_err ? KeyUpdateWipe : KeyUpdateKmac;
end
end
@@ -650,8 +648,8 @@
end
if (op_ack) begin
- op_update_sel = op_fault_err ? KeyUpdateWipe :
- disabled ? KeyUpdateInvalid : KeyUpdateKmac;
+ op_update_sel = disabled ? KeyUpdateInvalid :
+ op_fault_err ? KeyUpdateWipe : KeyUpdateKmac;
end
end
@@ -677,6 +675,7 @@
assign fault_o[FaultRegFileIntg] = regfile_intg_err_i;
assign fault_o[FaultShadow] = shadowed_err_i;
assign fault_o[FaultCtrlFsm] = state_intg_err_o;
+ assign fault_o[FaultCtrlCnt] = cnt_err;
always_comb begin
status_o = OpIdle;
diff --git a/hw/ip/keymgr/rtl/keymgr_kmac_if.sv b/hw/ip/keymgr/rtl/keymgr_kmac_if.sv
index 04e41bb..d6bd801 100644
--- a/hw/ip/keymgr/rtl/keymgr_kmac_if.sv
+++ b/hw/ip/keymgr/rtl/keymgr_kmac_if.sv
@@ -39,13 +39,38 @@
output logic cmd_error_o
);
- // Enumeration for working state
- typedef enum logic [2:0] {
- StIdle = 0,
- StTx = 1,
- StTxLast = 2,
- StOpWait = 3,
- StClean = 4
+
+ // Encoding generated with:
+ // $ ./util/design/sparse-fsm-encode.py -d 5 -m 6 -n 10 \
+ // -s 2292624416 --language=sv
+ //
+ // Hamming distance histogram:
+ //
+ // 0: --
+ // 1: --
+ // 2: --
+ // 3: --
+ // 4: --
+ // 5: |||||||||||||||||||| (46.67%)
+ // 6: ||||||||||||||||| (40.00%)
+ // 7: ||||| (13.33%)
+ // 8: --
+ // 9: --
+ // 10: --
+ //
+ // Minimum Hamming distance: 5
+ // Maximum Hamming distance: 7
+ // Minimum Hamming weight: 2
+ // Maximum Hamming weight: 9
+ //
+ localparam int StateWidth = 10;
+ typedef enum logic [StateWidth-1:0] {
+ StIdle = 10'b1110100010,
+ StTx = 10'b0010011011,
+ StTxLast = 10'b0101000000,
+ StOpWait = 10'b1000101001,
+ StClean = 10'b1111111101,
+ StInvalid = 10'b0011101110
} data_state_e;
localparam int AdvRem = AdvDataWidth % KmacDataIfWidth;
@@ -105,29 +130,43 @@
assign start = adv_en_i | id_en_i | gen_en_i;
- // downcount
- always_ff @(posedge clk_i or negedge rst_ni) begin
- if (!rst_ni) begin
- cnt <= '0;
- end else if (cnt_clr) begin
- cnt <= '0;
- end else if (cnt_set) begin
- cnt <= rounds;
- end else if (cnt_en && cnt >'0) begin
- cnt <= cnt - 1'b1;
- end
- end
+ logic cnt_err;
+ keymgr_cnt #(
+ .Width(CntWidth),
+ .OutSelDnCnt(1'b1)
+ ) u_cnt (
+ .clk_i,
+ .rst_ni,
+ .clr_i(cnt_clr),
+ .set_i(cnt_set),
+ .set_cnt_i(rounds),
+ .en_i(cnt_en),
+ .cnt_o(cnt),
+ .err_o(cnt_err)
+ );
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
inputs_invalid_q <= '0;
- state_q <= StIdle;
end else begin
inputs_invalid_q <= inputs_invalid_d;
- state_q <= state_d;
end
end
+ // This primitive is used to place a size-only constraint on the
+ // flops in order to prevent FSM state encoding optimizations.
+ logic [StateWidth-1:0] state_raw_q;
+ assign state_q = data_state_e'(state_raw_q);
+
+ prim_flop #(
+ .Width(StateWidth),
+ .ResetValue(StateWidth'(StIdle))
+ ) u_state_regs (
+ .clk_i,
+ .rst_ni,
+ .d_i ( state_d ),
+ .q_o ( state_raw_q )
+ );
always_comb begin
cnt_clr = 1'b0;
@@ -177,6 +216,11 @@
state_d = StTxLast;
end
end
+
+ if (cnt_err) begin
+ state_d = StInvalid;
+ end
+
end
StTxLast: begin
@@ -194,6 +238,10 @@
// transaction accepted
cnt_clr = kmac_data_i.ready;
state_d = kmac_data_i.ready ? StOpWait : StTxLast;
+
+ if (cnt_err) begin
+ state_d = StInvalid;
+ end
end
StOpWait: begin
diff --git a/hw/ip/keymgr/rtl/keymgr_pkg.sv b/hw/ip/keymgr/rtl/keymgr_pkg.sv
index 003144f..a257db4 100644
--- a/hw/ip/keymgr/rtl/keymgr_pkg.sv
+++ b/hw/ip/keymgr/rtl/keymgr_pkg.sv
@@ -103,6 +103,12 @@
Otbn
} keymgr_key_dest_e;
+ // Enumeration for hardened count style
+ typedef enum logic {
+ CrossCnt, // up count and down count
+ DupCnt // duplicate counters
+ } keymgr_cnt_style_e;
+
// Enumeration for key select
typedef enum logic {
HwKey = 0,
@@ -148,13 +154,14 @@
} keymgr_err_pos_e;
// Bit position of fault status
- typedef enum logic [2:0] {
+ typedef enum logic [3:0] {
FaultCmd,
FaultKmacFsm,
FaultKmacOp,
FaultRegFileIntg,
FaultShadow,
FaultCtrlFsm,
+ FaultCtrlCnt,
FaultLastPos
} keymgr_fault_pos_e;
diff --git a/hw/ip/keymgr/rtl/keymgr_reg_pkg.sv b/hw/ip/keymgr/rtl/keymgr_reg_pkg.sv
index c19b9bc..d1b94b4 100644
--- a/hw/ip/keymgr/rtl/keymgr_reg_pkg.sv
+++ b/hw/ip/keymgr/rtl/keymgr_reg_pkg.sv
@@ -214,6 +214,10 @@
logic d;
logic de;
} ctrl_fsm_intg;
+ struct packed {
+ logic d;
+ logic de;
+ } ctrl_fsm_cnt;
} keymgr_hw2reg_fault_status_reg_t;
// Register -> HW type
@@ -238,16 +242,16 @@
// HW -> register type
typedef struct packed {
- keymgr_hw2reg_intr_state_reg_t intr_state; // [562:561]
- keymgr_hw2reg_cfg_regwen_reg_t cfg_regwen; // [560:560]
- keymgr_hw2reg_control_reg_t control; // [559:558]
- keymgr_hw2reg_sw_binding_regwen_reg_t sw_binding_regwen; // [557:557]
- keymgr_hw2reg_sw_share0_output_mreg_t [7:0] sw_share0_output; // [556:293]
- keymgr_hw2reg_sw_share1_output_mreg_t [7:0] sw_share1_output; // [292:29]
- keymgr_hw2reg_working_state_reg_t working_state; // [28:25]
- keymgr_hw2reg_op_status_reg_t op_status; // [24:22]
- keymgr_hw2reg_err_code_reg_t err_code; // [21:12]
- keymgr_hw2reg_fault_status_reg_t fault_status; // [11:0]
+ keymgr_hw2reg_intr_state_reg_t intr_state; // [564:563]
+ keymgr_hw2reg_cfg_regwen_reg_t cfg_regwen; // [562:562]
+ keymgr_hw2reg_control_reg_t control; // [561:560]
+ keymgr_hw2reg_sw_binding_regwen_reg_t sw_binding_regwen; // [559:559]
+ keymgr_hw2reg_sw_share0_output_mreg_t [7:0] sw_share0_output; // [558:295]
+ keymgr_hw2reg_sw_share1_output_mreg_t [7:0] sw_share1_output; // [294:31]
+ keymgr_hw2reg_working_state_reg_t working_state; // [30:27]
+ keymgr_hw2reg_op_status_reg_t op_status; // [26:24]
+ keymgr_hw2reg_err_code_reg_t err_code; // [23:14]
+ keymgr_hw2reg_fault_status_reg_t fault_status; // [13:0]
} keymgr_hw2reg_t;
// Register offsets
diff --git a/hw/ip/keymgr/rtl/keymgr_reg_top.sv b/hw/ip/keymgr/rtl/keymgr_reg_top.sv
index e215c08..004e158 100644
--- a/hw/ip/keymgr/rtl/keymgr_reg_top.sv
+++ b/hw/ip/keymgr/rtl/keymgr_reg_top.sv
@@ -307,6 +307,7 @@
logic fault_status_regfile_intg_qs;
logic fault_status_shadow_qs;
logic fault_status_ctrl_fsm_intg_qs;
+ logic fault_status_ctrl_fsm_cnt_qs;
// Register instances
// R[intr_state]: V(False)
@@ -2248,6 +2249,32 @@
);
+ // F[ctrl_fsm_cnt]: 6:6
+ prim_subreg #(
+ .DW (1),
+ .SwAccess(prim_subreg_pkg::SwAccessRO),
+ .RESVAL (1'h0)
+ ) u_fault_status_ctrl_fsm_cnt (
+ .clk_i (clk_i),
+ .rst_ni (rst_ni),
+
+ // from register interface
+ .we (1'b0),
+ .wd ('0),
+
+ // from internal hardware
+ .de (hw2reg.fault_status.ctrl_fsm_cnt.de),
+ .d (hw2reg.fault_status.ctrl_fsm_cnt.d),
+
+ // to internal hardware
+ .qe (),
+ .q (),
+
+ // to register interface (read)
+ .qs (fault_status_ctrl_fsm_cnt_qs)
+ );
+
+
logic [59:0] addr_hit;
@@ -2830,6 +2857,7 @@
reg_rdata_next[3] = fault_status_regfile_intg_qs;
reg_rdata_next[4] = fault_status_shadow_qs;
reg_rdata_next[5] = fault_status_ctrl_fsm_intg_qs;
+ reg_rdata_next[6] = fault_status_ctrl_fsm_cnt_qs;
end
default: begin
