[rom_ctrl] Make the mux select into a MuBi4
This strengthens the "did we switch back to the checker?" logic.
Firstly, we've got sel_reverted which corresponds to the check that
was in alert_o before. However, we also take a copy of sel_bus_q and
check for reversion *there*. The idea is that if an attacker somehow
managed to override sel_buf_q, there would be yet another check to
catch it.
The sel_bus_i signal that tells the mux to switch to giving access to
the bus comes from rom_ctrl_fsm and boils down to a check that the FSM
is in state Done. We bulk out the sparse encoding for that FSM by
adding the MuBi4 value that compares the state with Done. That way, we
can pass those bits straight through to the mux without ever having a
glitchable 1-bit intermediate.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/rom_ctrl/rom_ctrl.core b/hw/ip/rom_ctrl/rom_ctrl.core
index db219f1..42ee5f4 100644
--- a/hw/ip/rom_ctrl/rom_ctrl.core
+++ b/hw/ip/rom_ctrl/rom_ctrl.core
@@ -10,6 +10,7 @@
depend:
- lowrisc:prim:alert
- lowrisc:prim:assert
+ - lowrisc:prim:buf
- lowrisc:prim:cipher
- lowrisc:prim:rom_adv
- lowrisc:prim:subreg
diff --git a/hw/ip/rom_ctrl/rtl/rom_ctrl.sv b/hw/ip/rom_ctrl/rtl/rom_ctrl.sv
index 8c41693..ac8a885 100644
--- a/hw/ip/rom_ctrl/rtl/rom_ctrl.sv
+++ b/hw/ip/rom_ctrl/rtl/rom_ctrl.sv
@@ -43,6 +43,7 @@
import rom_ctrl_pkg::*;
import rom_ctrl_reg_pkg::*;
+ import prim_mubi_pkg::mubi4_t, prim_mubi_pkg::MuBi4True;
import prim_util_pkg::vbits;
// ROM_CTRL_ROM_SIZE is auto-generated by regtool and comes from the bus window size, measured in
@@ -56,7 +57,7 @@
// bits on the fly.
localparam int unsigned DataWidth = SecDisableScrambling ? 32 : 39;
- logic rom_select;
+ mubi4_t rom_select_bus;
logic [RomIndexWidth-1:0] rom_index;
logic rom_req;
@@ -185,7 +186,7 @@
) u_mux (
.clk_i (clk_i),
.rst_ni (rst_ni),
- .sel_i (rom_select),
+ .sel_bus_i (rom_select_bus),
.bus_addr_i (bus_rom_index),
.bus_req_i (bus_rom_req),
.bus_gnt_o (bus_rom_gnt),
@@ -301,7 +302,7 @@
.kmac_rom_last_o (kmac_rom_last),
.kmac_done_i (kmac_done),
.kmac_digest_i (kmac_digest),
- .rom_select_o (rom_select),
+ .rom_select_bus_o (rom_select_bus),
.rom_addr_o (checker_rom_index),
.rom_req_o (checker_rom_req),
.rom_data_i (checker_rom_rdata[31:0]),
@@ -327,9 +328,9 @@
assign kmac_rom_vld = 1'b0;
assign kmac_rom_last = 1'b0;
- // select = 0 => grant access to the bus. Setting this to a constant should mean the mux gets
+ // Always grant access to the bus. Setting this to a constant should mean the mux gets
// synthesized away completely.
- assign rom_select = 1'b0;
+ assign rom_select_bus = MuBi4True;
assign checker_rom_index = '0;
assign checker_rom_req = 1'b0;
diff --git a/hw/ip/rom_ctrl/rtl/rom_ctrl_fsm.sv b/hw/ip/rom_ctrl/rtl/rom_ctrl_fsm.sv
index c50b665..4362da0 100644
--- a/hw/ip/rom_ctrl/rtl/rom_ctrl_fsm.sv
+++ b/hw/ip/rom_ctrl/rtl/rom_ctrl_fsm.sv
@@ -7,6 +7,7 @@
//
module rom_ctrl_fsm
+ import prim_mubi_pkg::mubi4_t;
import prim_util_pkg::vbits;
#(
parameter int RomDepth = 16,
@@ -42,7 +43,7 @@
input logic [TopCount*32-1:0] kmac_digest_i,
// To ROM mux
- output logic rom_select_o,
+ output mubi4_t rom_select_bus_o,
output logic [vbits(RomDepth)-1:0] rom_addr_o,
output logic rom_req_o,
@@ -54,6 +55,8 @@
);
import prim_mubi_pkg::mubi4_bool_to_mubi;
+ import prim_mubi_pkg::mubi4_test_true_loose;
+ import prim_mubi_pkg::MuBi4False, prim_mubi_pkg::MuBi4True;
localparam int AW = vbits(RomDepth);
localparam int TAW = vbits(TopCount);
@@ -123,7 +126,7 @@
// Done [peripheries=2];
// }
//
- // Encoding generated with:
+ // Initial encoding generated with:
// $ util/design/sparse-fsm-encode.py -d 3 -m 6 -n 6 -s 2 --language=sv
//
// Hamming distance histogram:
@@ -141,19 +144,23 @@
// Minimum Hamming weight: 1
// Maximum Hamming weight: 5
//
- typedef enum logic [5:0] {
- ReadingLow = 6'b111101,
- ReadingHigh = 6'b110110,
- RomAhead = 6'b000011,
- KmacAhead = 6'b101010,
- Checking = 6'b010000,
- Done = 6'b001100
+ // However, we glom on an extra 4 bits to hold a mubi4_t that encodes "state == Done". The idea is
+ // that we can do that for the rom_select_bus_o signal without needing an intermediate 1-bit
+ // signal which would need burying.
+ //
+ typedef enum logic [9:0] {
+ ReadingLow = {6'b111101, MuBi4False},
+ ReadingHigh = {6'b110110, MuBi4False},
+ RomAhead = {6'b000011, MuBi4False},
+ KmacAhead = {6'b101010, MuBi4False},
+ Checking = {6'b010000, MuBi4False},
+ Done = {6'b001100, MuBi4True}
} state_e;
- logic [5:0] state_q, state_d;
+ logic [9:0] state_q, state_d;
logic fsm_alert;
- prim_flop #(.Width(6), .ResetValue({ReadingLow}))
+ prim_flop #(.Width(10), .ResetValue({ReadingLow}))
u_state_regs (
.clk_i (clk_i),
.rst_ni (rst_ni),
@@ -206,6 +213,12 @@
endcase
end
+ // The in_state_done signal is supposed to be true iff we're in FSM state Done. Grabbing just the
+ // bottom 4 bits of state_q is equivalent to "mubi4_bool_to_mubi(state_q == Done)" except that it
+ // doesn't have a 1-bit signal on the way.
+ mubi4_t in_state_done;
+ assign in_state_done = mubi4_t'(state_q[3:0]);
+
// Route digest signals coming back from KMAC straight to the CSRs
assign digest_o = kmac_digest_i;
assign digest_vld_o = kmac_done_i;
@@ -229,12 +242,14 @@
assign exp_digest_vld_o = reading_top;
assign exp_digest_idx_o = rel_addr;
- // Pass the 'done' and 'good' signals directly from the checker
- assign pwrmgr_data_o = '{done: mubi4_bool_to_mubi(state_q == Done),
+ // The 'done' signal for pwrmgr is asserted once we get into the Done state. The 'good' signal
+ // compes directly from the checker.
+ assign pwrmgr_data_o = '{done: in_state_done,
good: mubi4_bool_to_mubi(checker_good)};
- // Pass the digest all-at-once to the keymgr
- assign keymgr_data_o = '{data: digest_i, valid: (state_q == Done)};
+ // Pass the digest all-at-once to the keymgr. The loose check means that glitches will add
+ // spurious edges to the valid signal that can be caught at the other end.
+ assign keymgr_data_o = '{data: digest_i, valid: mubi4_test_true_loose(in_state_done)};
// KMAC rom data interface
//
@@ -254,8 +269,9 @@
end
end
- // We keep control of the ROM mux from reset until we're done
- assign rom_select_o = (state_q != Done);
+ // We keep control of the ROM mux from reset until we're done.
+ assign rom_select_bus_o = in_state_done;
+
assign rom_addr_o = counter_read_addr;
assign rom_req_o = counter_read_req;
diff --git a/hw/ip/rom_ctrl/rtl/rom_ctrl_mux.sv b/hw/ip/rom_ctrl/rtl/rom_ctrl_mux.sv
index 10d5ef8..c85066a 100644
--- a/hw/ip/rom_ctrl/rtl/rom_ctrl_mux.sv
+++ b/hw/ip/rom_ctrl/rtl/rom_ctrl_mux.sv
@@ -6,15 +6,19 @@
// The mux to select between ROM inputs
//
-module rom_ctrl_mux #(
+module rom_ctrl_mux
+ import prim_mubi_pkg::mubi4_t;
+#(
parameter int AW = 8,
parameter int DW = 39
) (
input logic clk_i,
input logic rst_ni,
- // select signal. 1 = checker; 0 = bus
- input logic sel_i,
+ // Select signal saying whether access is granted to the bus. This module raises an alert (by
+ // setting alert_o) if the signal isn't an allowed value or if the selection switches back from
+ // the bus to the checker.
+ input mubi4_t sel_bus_i,
// Interface for bus
input logic [AW-1:0] bus_addr_i,
@@ -36,32 +40,63 @@
input logic rom_rvalid_i,
// Alert output
+ //
+ // This isn't latched in this module because it feeds into a fatal alert at top-level, whose
+ // sender will latch it anyway.
output logic alert_o
);
- // TODO: sel_q will definitely need to be multi-bit for glitch resistance. We'll probably also
- // have to chase the "signal bit signals" back a bit further through the logic too.
- logic sel_q;
- always_ff @(posedge clk_i or negedge rst_ni) begin
- if (!rst_ni) begin
- sel_q <= 1'b1;
- end else begin
- sel_q <= sel_q & sel_i;
- end
- end
+ import prim_mubi_pkg::*;
- // Spot if the select signal becomes one again after it went to zero
- assign alert_o = sel_i & ~sel_q;
+ // Track the state of the mux up to the current cycle. This is a "1-way" mux, which means that
+ // we never switch from the bus back to the checker.
+ //
+ // We also have a version that's delayed by a single cycle to allow a check that sel_bus_q is
+ // never reset from True to False.
+ mubi4_t sel_bus_q, sel_bus_qq;
- // The bus can have access every cycle, once the select signal is zero.
- assign bus_gnt_o = ~sel_i;
+ prim_flop #(.Width (4), .ResetValue ({MuBi4False}))
+ u_sel_bus_q_flop (
+ .clk_i,
+ .rst_ni,
+ .d_i (mubi4_or_hi(sel_bus_q, sel_bus_i)),
+ .q_o (sel_bus_q)
+ );
+
+ prim_flop #(.Width (4), .ResetValue ({MuBi4False}))
+ u_sel_bus_qq_flop (
+ .clk_i,
+ .rst_ni,
+ .d_i (sel_bus_q),
+ .q_o (sel_bus_qq)
+ );
+
+ // Spot if the sel_bus_i signal or its register version has a corrupt value.
+ logic sel_invalid;
+ assign sel_invalid = mubi4_test_invalid(sel_bus_i) || mubi4_test_invalid(sel_bus_q);
+
+ // Spot if the select signal switches back to the checker once we've switched to the bus. Doing so
+ // will have no lasting effect because of how we calculate sel_bus_q) but isn't supposed to
+ // happen, so we want to trigger an alert.
+ logic sel_reverted;
+ assign sel_reverted = mubi4_test_true_loose(sel_bus_q) & mubi4_test_false_loose(sel_bus_i);
+
+ // Spot if the sel_bus_q signal has reverted somehow.
+ logic sel_q_reverted;
+ assign sel_q_reverted = mubi4_test_true_loose(sel_bus_qq) & mubi4_test_false_loose(sel_bus_q);
+
+ assign alert_o = sel_invalid | sel_reverted | sel_q_reverted;
+
+ // The bus can have access every cycle, from when the select signal switches to the bus.
+ assign bus_gnt_o = mubi4_test_true_strict(sel_bus_i);
assign bus_rdata_o = rom_clr_rdata_i;
- // A high rom_rvalid_i is a response to a bus request if sel_i was zero on the previous cycle.
- assign bus_rvalid_o = ~sel_q & rom_rvalid_i;
+ // A high rom_rvalid_i is a response to a bus request if the select signal pointed at the bus on
+ // the previous cycle.
+ assign bus_rvalid_o = mubi4_test_true_strict(sel_bus_q) & rom_rvalid_i;
assign chk_rdata_o = rom_scr_rdata_i;
- assign rom_addr_o = sel_i ? chk_addr_i : bus_addr_i;
- assign rom_req_o = sel_i ? chk_req_i : bus_req_i;
+ assign rom_addr_o = mubi4_test_true_strict(sel_bus_i) ? bus_addr_i : chk_addr_i;
+ assign rom_req_o = mubi4_test_true_strict(sel_bus_i) ? bus_req_i : chk_req_i;
endmodule
