[keymgr] Enhance checks
- update data checks
- add second enable to control datapath to sideload and software
- directly feed stored key state out instead of storing into sideload
Signed-off-by: Timothy Chen <timothytim@google.com>
diff --git a/hw/ip/keymgr/rtl/keymgr.sv b/hw/ip/keymgr/rtl/keymgr.sv
index 023099d..2759489 100644
--- a/hw/ip/keymgr/rtl/keymgr.sv
+++ b/hw/ip/keymgr/rtl/keymgr.sv
@@ -140,9 +140,10 @@
logic adv_en, id_en, gen_en;
logic load_key;
logic wipe_key;
- logic [Shares-1:0][KeyWidth-1:0] kmac_key;
+ hw_key_req_t kmac_key;
logic op_done;
logic data_valid;
+ logic data_en;
logic kmac_done;
logic kmac_input_invalid;
logic kmac_cmd_err;
@@ -166,6 +167,7 @@
.sw_binding_unlock_o(sw_binding_unlock),
.status_o(hw2reg.op_status.d),
.error_o(err_code),
+ .data_en_o(data_en),
.data_valid_o(data_valid),
.working_state_o(hw2reg.working_state.d),
.root_key_i(otp_key_i),
@@ -213,6 +215,7 @@
// The various arrays of inputs for each operation
logic [2**StageWidth-1:0][AdvDataWidth-1:0] adv_matrix;
+ logic [2**StageWidth-1:0] adv_dvalid;
logic [2**StageWidth-1:0][IdDataWidth-1:0] id_matrix;
logic [GenDataWidth-1:0] gen_in;
@@ -224,12 +227,18 @@
localparam int IdLfsrCopies = IdDataWidth / 32;
localparam int GenLfsrCopies = GenDataWidth / 32;
- // input checking - currently only MAX version, others TBD
- logic key_version_good;
+ // input checking
+ logic creator_seed_vld;
+ logic owner_seed_vld;
+ logic devid_vld;
+ logic health_state_vld;
+ logic key_version_vld;
+
// Advance state operation input construction
for (genvar i = KeyMgrStages; i < 2**StageWidth; i++) begin : gen_adv_matrix_fill
assign adv_matrix[i] = {AdvLfsrCopies{lfsr[31:0]}};
+ assign adv_dvalid[i] = 1'b0;
end
// Advance to creator_root_key
@@ -241,14 +250,19 @@
lc_i.health_state,
creator_seed});
+ assign adv_dvalid[Creator] = creator_seed_vld &
+ devid_vld &
+ health_state_vld;
+
// Advance to owner_intermediate_key
logic [KeyWidth-1:0] owner_seed;
assign owner_seed = flash_i.seeds[flash_ctrl_pkg::OwnerSeedIdx];
assign adv_matrix[OwnerInt] = AdvDataWidth'({reg2hw.sw_binding,owner_seed});
+ assign adv_dvalid[OwnerInt] = owner_seed_vld;
// Advance to owner_key
- assign adv_matrix[Owner] = AdvDataWidth'(reg2hw.sw_binding);
-
+ assign adv_matrix[Owner] = AdvDataWidth'(reg2hw.sw_binding);
+ assign adv_dvalid[Owner] = 1'b1;
// Generate Identity operation input construction
for (genvar i = KeyMgrStages; i < 2**StageWidth; i++) begin : gen_id_matrix_fill
@@ -279,11 +293,22 @@
// General module for checking inputs
+ logic key_vld;
keymgr_input_checks u_checks (
.max_key_versions_i(max_key_versions),
.stage_sel_i(stage_sel),
.key_version_i(reg2hw.key_version),
- .key_version_good_o(key_version_good)
+ .creator_seed_i(creator_seed),
+ .owner_seed_i(owner_seed),
+ .key_i(kmac_key_o),
+ .devid_i(otp_i.devid),
+ .health_state_i(lc_i.health_state),
+ .creator_seed_vld_o(creator_seed_vld),
+ .owner_seed_vld_o(owner_seed_vld),
+ .devid_vld_o(devid_vld),
+ .health_state_vld_o(health_state_vld),
+ .key_version_vld_o(key_version_vld),
+ .key_vld_o(key_vld)
);
@@ -291,15 +316,11 @@
// KMAC Control
/////////////////////////////////////
- logic key_version_err;
logic [3:0] invalid_data;
-
- assign key_version_err = !key_version_good;
-
- assign invalid_data[OpAdvance] = '0; // TBD
- assign invalid_data[OpGenId] = '0; // TBD
- assign invalid_data[OpGenSwOut] = key_version_err; // TBD, more to come
- assign invalid_data[OpGenHwOut] = key_version_err; // TBD, more to come
+ assign invalid_data[OpAdvance] = ~key_vld | ~adv_dvalid[stage_sel];
+ assign invalid_data[OpGenId] = ~key_vld;
+ assign invalid_data[OpGenSwOut] = ~key_vld | ~key_version_vld;
+ assign invalid_data[OpGenHwOut] = ~key_vld | ~key_version_vld;
keymgr_kmac_if u_kmac_if (
.clk_i,
@@ -339,6 +360,7 @@
.dest_sel_i(keymgr_key_dest_e'(reg2hw.control.dest_sel)),
.key_sel_i(key_sel),
.load_key_i(load_key),
+ .data_en_i(data_en),
.data_valid_i(data_valid),
.key_i(kmac_key),
.data_i(kmac_data),
@@ -349,8 +371,10 @@
);
for (genvar i = 0; i < 8; i++) begin : gen_sw_assigns
- assign hw2reg.sw_share0_output[i].d = wipe_key ? key_entropy : kmac_data[0][i*32 +: 32];
- assign hw2reg.sw_share1_output[i].d = wipe_key ? key_entropy : kmac_data[1][i*32 +: 32];
+ assign hw2reg.sw_share0_output[i].d = ~data_en | wipe_key ? key_entropy :
+ kmac_data[0][i*32 +: 32];
+ assign hw2reg.sw_share1_output[i].d = ~data_en | wipe_key ? key_entropy :
+ kmac_data[1][i*32 +: 32];
assign hw2reg.sw_share0_output[i].de = wipe_key | data_valid & (key_sel == SwKey);
assign hw2reg.sw_share1_output[i].de = wipe_key | data_valid & (key_sel == SwKey);
end
diff --git a/hw/ip/keymgr/rtl/keymgr_ctrl.sv b/hw/ip/keymgr/rtl/keymgr_ctrl.sv
index 112000a..68bdd34 100644
--- a/hw/ip/keymgr/rtl/keymgr_ctrl.sv
+++ b/hw/ip/keymgr/rtl/keymgr_ctrl.sv
@@ -20,6 +20,7 @@
output logic op_done_o,
output keymgr_op_status_e status_o,
output logic [ErrLastPos-1:0] error_o,
+ output logic data_en_o,
output logic data_valid_o,
output logic wipe_key_o,
output keymgr_working_state_e working_state_o,
@@ -34,7 +35,7 @@
output logic adv_en_o,
output logic id_en_o,
output logic gen_en_o,
- output logic [Shares-1:0][KeyWidth-1:0] key_o,
+ output hw_key_req_t key_o,
output logic load_key_o,
input kmac_done_i,
input kmac_input_invalid_i, // asserted when selected data fails criteria check
@@ -74,7 +75,6 @@
logic cnt_en;
logic cnt_clr;
logic data_valid;
- logic adv_en_q;
logic op_accepted;
logic invalid_op;
@@ -105,7 +105,7 @@
assign adv_en_o = op_accepted & (advance_sel | disable_sel);
assign id_en_o = op_accepted & gen_id_sel;
assign gen_en_o = op_accepted & gen_out_sel;
- assign load_key_o = adv_en_o & !adv_en_q;
+ assign load_key_o = op_accepted;
// unlock sw binding configuration whenever an advance call is made without errors
assign sw_binding_unlock_o = adv_en_o & op_done_o & ~|error_o;
@@ -119,10 +119,8 @@
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
state_q <= StCtrlReset;
- adv_en_q <= 1'b0;
end else begin
state_q <= state_d;
- adv_en_q <= adv_en_o;
end
end
@@ -130,7 +128,11 @@
// - whatever operation causes the input data select to be disabled should not expose the key
// state.
// - when there are no operations, the key state also should be exposed.
- assign key_o = (~op_start_i | disable_sel) ? {EntropyRounds * Shares {entropy_i}} : key_state_q;
+ assign key_o.valid = op_accepted;
+ assign key_o.key_share0 = (~op_start_i | disable_sel) ? {EntropyRounds{entropy_i}} :
+ key_state_q[0];
+ assign key_o.key_share1 = (~op_start_i | disable_sel) ? {EntropyRounds{entropy_i}} :
+ key_state_q[1];
// key state is intentionally not reset
always_ff @(posedge clk_i) begin
@@ -413,6 +415,8 @@
// if operation was never accepted (ie a generate was called in StReset / StRandom), then
// never update the sw / hw outputs when operation is complete
+ // TODO: This is a critical single point of failure, need to think deeply about how to
+ // enhance this.
assign data_valid_o = op_done_o & op_accepted & data_valid & gen_sel;
// data errors are not relevant when operation was not accepted.
@@ -431,6 +435,78 @@
end
+ ///////////////////////////////
+ // Suppress kmac return data
+ ///////////////////////////////
+ // This is a separate data path from the FSM used to control the data_en_o output
+
+ typedef enum logic [1:0] {
+ StCtrlDataIdle,
+ StCtrlDataEn,
+ StCtrlDataDis,
+ StCtrlDataWait
+ } keymgr_ctrl_data_state_e;
+
+ keymgr_ctrl_data_state_e data_st_d, data_st_q;
+
+ always_ff @(posedge clk_i or negedge rst_ni) begin
+ if (!rst_ni) begin
+ data_st_q <= StCtrlDataIdle;
+ end else begin
+ data_st_q <= data_st_d;
+ end
+ end
+
+ // The below control path is used for modulating the datapath to sideload and sw keys.
+ // This path is separate from the data_valid_o path, thus creating two separate attack points.
+ // The data is only enabled when a non-advance operation is invoked.
+ // When an advance operation is called, the data is disabled. It will stay disabled until an
+ // entire completion sequence is seen (op_done_o assert -> start_i de-assertion).
+ // When a generate operation is called, the data is enabled. However, any indication of this
+ // supposedly being an advance call will force the path to disable again.
+ always_comb begin
+ data_st_d = data_st_q;
+ data_en_o = 1'b0;
+ unique case (data_st_q)
+
+ StCtrlDataIdle: begin
+ if (adv_en_o) begin
+ data_st_d = StCtrlDataDis;
+ end else if (id_en_o || gen_en_o) begin
+ data_st_d = StCtrlDataEn;
+ end
+ end
+
+ StCtrlDataEn: begin
+ data_en_o = 1'b1;
+ if (adv_en_o) begin
+ data_st_d = StCtrlDataDis;
+ end
+ end
+
+ StCtrlDataDis: begin
+ if (op_done_o) begin
+ data_st_d = StCtrlDataWait;
+ end
+ end
+
+ StCtrlDataWait: begin
+ if (!op_start_i) begin
+ data_st_d = StCtrlDataIdle;
+ end
+ end
+
+ default:;
+
+ endcase // unique case (data_st_q)
+ end
+
+
+
+
+
+
+
///////////////////////////////
// Functions
diff --git a/hw/ip/keymgr/rtl/keymgr_input_checks.sv b/hw/ip/keymgr/rtl/keymgr_input_checks.sv
index 1a0222e..ab1da17 100644
--- a/hw/ip/keymgr/rtl/keymgr_input_checks.sv
+++ b/hw/ip/keymgr/rtl/keymgr_input_checks.sv
@@ -11,16 +11,44 @@
module keymgr_input_checks import keymgr_pkg::*;(
input [2**StageWidth-1:0][31:0] max_key_versions_i,
input keymgr_stage_e stage_sel_i,
+ input hw_key_req_t key_i,
input [31:0] key_version_i,
- output logic key_version_good_o
+ input [KeyWidth-1:0] creator_seed_i,
+ input [KeyWidth-1:0] owner_seed_i,
+ input [DevIdWidth-1:0] devid_i,
+ input [HealthStateWidth-1:0] health_state_i,
+ output logic creator_seed_vld_o,
+ output logic owner_seed_vld_o,
+ output logic devid_vld_o,
+ output logic health_state_vld_o,
+ output logic key_version_vld_o,
+ output logic key_vld_o
);
logic [31:0] cur_max_key_version;
assign cur_max_key_version = max_key_versions_i[stage_sel_i];
// key version must be smaller than or equal to max version
- assign key_version_good_o = key_version_i <= cur_max_key_version;
+ assign key_version_vld_o = key_version_i <= cur_max_key_version;
+ // general data check
+ assign creator_seed_vld_o = valid_chk(MaxWidth'(creator_seed_i));
+ assign owner_seed_vld_o = valid_chk(MaxWidth'(owner_seed_i));
+ assign devid_vld_o = valid_chk(MaxWidth'(devid_i));
+ assign health_state_vld_o = valid_chk(MaxWidth'(health_state_i));
+
+ // key check
+ logic unused_key_vld;
+ assign unused_key_vld = key_i.valid;
+ assign key_vld_o = valid_chk(MaxWidth'(key_i.key_share0)) &
+ valid_chk(MaxWidth'(key_i.key_share1));
+
+ // checks for all 0's or all 1's of value
+ function automatic logic valid_chk (logic [MaxWidth-1:0] value);
+
+ return |value & ~&value;
+
+ endfunction // valid_chk
endmodule // keymgr_input_checks
diff --git a/hw/ip/keymgr/rtl/keymgr_kmac_if.sv b/hw/ip/keymgr/rtl/keymgr_kmac_if.sv
index 414388a..167d443 100644
--- a/hw/ip/keymgr/rtl/keymgr_kmac_if.sv
+++ b/hw/ip/keymgr/rtl/keymgr_kmac_if.sv
@@ -15,7 +15,7 @@
input [AdvDataWidth-1:0] adv_data_i,
input [IdDataWidth-1:0] id_data_i,
input [GenDataWidth-1:0] gen_data_i,
- input [3:0] inputs_invalid_i, // probably should break down into categories later
+ input [3:0] inputs_invalid_i,
output logic inputs_invalid_o,
// keymgr control to select appropriate inputs
diff --git a/hw/ip/keymgr/rtl/keymgr_pkg.sv b/hw/ip/keymgr/rtl/keymgr_pkg.sv
index 92d3a90..5444f1f 100644
--- a/hw/ip/keymgr/rtl/keymgr_pkg.sv
+++ b/hw/ip/keymgr/rtl/keymgr_pkg.sv
@@ -18,6 +18,7 @@
// These should be defined in another module's package
parameter int HealthStateWidth = 128;
parameter int DevIdWidth = 256;
+ parameter int MaxWidth = 256;
// Default seeds
// These are generated using random.org byte dumper
diff --git a/hw/ip/keymgr/rtl/keymgr_sideload_key.sv b/hw/ip/keymgr/rtl/keymgr_sideload_key.sv
index d69cb1a..d2d3e94 100644
--- a/hw/ip/keymgr/rtl/keymgr_sideload_key.sv
+++ b/hw/ip/keymgr/rtl/keymgr_sideload_key.sv
@@ -10,6 +10,7 @@
input clk_i,
input rst_ni,
input en_i,
+ input set_en_i,
input set_i,
input clr_i,
input [31:0] entropy_i,
@@ -42,7 +43,9 @@
key_q[i] <= {EntropyCopies{entropy_i}};
end
end else if (set_i) begin
- key_q <= key_i;
+ for (int i = 0; i < Shares; i++) begin
+ key_q[i] <= set_en_i ? key_i[i] : {EntropyCopies{entropy_i}};
+ end
end
end
diff --git a/hw/ip/keymgr/rtl/keymgr_sideload_key_ctrl.sv b/hw/ip/keymgr/rtl/keymgr_sideload_key_ctrl.sv
index 95b0825..0fb9b35 100644
--- a/hw/ip/keymgr/rtl/keymgr_sideload_key_ctrl.sv
+++ b/hw/ip/keymgr/rtl/keymgr_sideload_key_ctrl.sv
@@ -15,8 +15,9 @@
input keymgr_key_dest_e dest_sel_i,
input keymgr_gen_out_e key_sel_i,
input load_key_i,
+ input data_en_i,
input data_valid_i,
- input [Shares-1:0][KeyWidth-1:0] key_i,
+ input hw_key_req_t key_i,
input [Shares-1:0][KeyWidth-1:0] data_i,
output logic prng_en_o,
output hw_key_req_t aes_key_o,
@@ -100,6 +101,7 @@
.clk_i,
.rst_ni,
.en_i(keys_en),
+ .set_en_i(data_en_i),
.set_i(data_valid_i & aes_sel),
.clr_i(clr), // TBD, should add an option for software clear later
.entropy_i(entropy_i),
@@ -111,6 +113,7 @@
.clk_i,
.rst_ni,
.en_i(keys_en),
+ .set_en_i(data_en_i),
.set_i(data_valid_i & hmac_sel),
.clr_i(clr), // TBD, should add an option for software clear later
.entropy_i(entropy_i),
@@ -118,17 +121,22 @@
.key_o(hmac_key_o)
);
+ hw_key_req_t kmac_sideload_key;
keymgr_sideload_key u_kmac_key (
.clk_i,
.rst_ni,
.en_i(keys_en),
- .set_i(load_key_i | (data_valid_i & kmac_sel)),
+ .set_en_i(data_en_i),
+ .set_i(data_valid_i & kmac_sel),
.clr_i(clr), // TBD, should add an option for software clear laterclr
.entropy_i(entropy_i),
- .key_i(load_key_i ? key_i : data_i),
- .key_o(kmac_key_o)
+ .key_i(data_i),
+ .key_o(kmac_sideload_key)
);
+ // when directed by keymgr_ctrl, switch over to internal key and feed to kmac
+ assign kmac_key_o = load_key_i ? key_i : kmac_sideload_key;
+
// when clearing, request prng
assign prng_en_o = clr;
