From 16c514e4239e872a2bd370e17ab7d5e2205d1acc Mon Sep 17 00:00:00 2001 From: Pascal Nasahl Date: Fri, 22 Dec 2023 12:03:14 +0100 Subject: [PATCH] [rtl] Fix FI vulnerability in RF As described in #20715, a single fault-induced bit-flip inside the register file could change which of the register file value is provided to Ibex. This PR fixes this issue by (i) encoding raddr_a/b to one-hot encoded signals, (ii) checking these signals for faults, and (iii) using an one-hot encoded MUX to select which register file value is forwarded to rdata_a/b. Area increases by ~1% (Yosys + Nangate45 synthesis). I conducted a formal fault injection verification at the Yosys netlist to ensure that the issue really is fixed. Signed-off-by: Pascal Nasahl --- rtl/ibex_register_file_ff.sv | 107 ++++++++++++++++++++++++++++-- rtl/ibex_register_file_fpga.sv | 113 ++++++++++++++++++++++++++++++-- rtl/ibex_register_file_latch.sv | 107 ++++++++++++++++++++++++++++-- rtl/ibex_top.sv | 20 +++--- 4 files changed, 322 insertions(+), 25 deletions(-) diff --git a/rtl/ibex_register_file_ff.sv b/rtl/ibex_register_file_ff.sv index 45890db886..dfcfd97957 100644 --- a/rtl/ibex_register_file_ff.sv +++ b/rtl/ibex_register_file_ff.sv @@ -15,6 +15,7 @@ module ibex_register_file_ff #( parameter int unsigned DataWidth = 32, parameter bit DummyInstructions = 0, parameter bit WrenCheck = 0, + parameter bit RdataMuxCheck = 0, parameter logic [DataWidth-1:0] WordZeroVal = '0 ) ( // Clock and Reset @@ -39,7 +40,7 @@ module ibex_register_file_ff #( input logic [DataWidth-1:0] wdata_a_i, input logic we_a_i, - // This indicates whether spurious WE are detected. + // This indicates whether spurious WE or one-hot encoded raddr are detected. output logic err_o ); @@ -49,6 +50,11 @@ module ibex_register_file_ff #( logic [DataWidth-1:0] rf_reg [NUM_WORDS]; logic [NUM_WORDS-1:0] we_a_dec; + logic [NUM_WORDS-1:0] raddr_onehot_a, raddr_onehot_b; + logic [NUM_WORDS-1:0] raddr_onehot_a_buf, raddr_onehot_b_buf; + + logic oh_raddr_a_err, oh_raddr_b_err, oh_we_err; + always_comb begin : we_a_decoder for (int unsigned i = 0; i < NUM_WORDS; i++) begin we_a_dec[i] = (waddr_a_i == 5'(i)) ? we_a_i : 1'b0; @@ -78,12 +84,12 @@ module ibex_register_file_ff #( .oh_i(we_a_dec_buf), .addr_i(waddr_a_i), .en_i(we_a_i), - .err_o + .err_o(oh_we_err) ); end else begin : gen_no_wren_check logic unused_strobe; assign unused_strobe = we_a_dec[0]; // this is never read from in this case - assign err_o = 1'b0; + assign oh_we_err = 1'b0; end // No flops for R0 as it's hard-wired to 0 @@ -130,8 +136,99 @@ module ibex_register_file_ff #( assign rf_reg[0] = WordZeroVal; end - assign rdata_a_o = rf_reg[raddr_a_i]; - assign rdata_b_o = rf_reg[raddr_b_i]; + if (RdataMuxCheck) begin : gen_rdata_mux_check + // Encode raddr_a/b into one-hot encoded signals. + prim_onehot_enc #( + .OneHotWidth(NUM_WORDS) + ) u_prim_onehot_enc_raddr_a ( + .in_i (raddr_a_i), + .en_i (1'b1), + .out_o (raddr_onehot_a) + ); + + prim_onehot_enc #( + .OneHotWidth(NUM_WORDS) + ) u_prim_onehot_enc_raddr_b ( + .in_i (raddr_b_i), + .en_i (1'b1), + .out_o (raddr_onehot_b) + ); + + // Buffer the one-hot encoded signals so that the checkers + // are not optimized. + prim_buf #( + .Width(NUM_WORDS) + ) u_prim_buf_raddr_a ( + .in_i (raddr_onehot_a), + .out_o(raddr_onehot_a_buf) + ); + + prim_buf #( + .Width(NUM_WORDS) + ) u_prim_buf_raddr_b ( + .in_i (raddr_onehot_b), + .out_o(raddr_onehot_b_buf) + ); + + // Check the one-hot encoded signals for glitches. + prim_onehot_check #( + .AddrWidth(ADDR_WIDTH), + .OneHotWidth(NUM_WORDS), + .AddrCheck(1), + .EnableCheck(0) + ) u_prim_onehot_check_raddr_a ( + .clk_i, + .rst_ni, + .oh_i (raddr_onehot_a_buf), + .addr_i (raddr_a_i), + .en_i (1'b0), + .err_o (oh_raddr_a_err) + ); + + prim_onehot_check #( + .AddrWidth(ADDR_WIDTH), + .OneHotWidth(NUM_WORDS), + .AddrCheck(1), + .EnableCheck(0) + ) u_prim_onehot_check_raddr_b ( + .clk_i, + .rst_ni, + .oh_i (raddr_onehot_b_buf), + .addr_i (raddr_b_i), + .en_i (1'b0), + .err_o (oh_raddr_b_err) + ); + + // MUX register to rdata_a/b_o according to raddr_a/b_onehot. + prim_onehot_mux #( + .Width(DataWidth), + .Inputs(NUM_WORDS) + ) u_rdata_a_mux ( + .clk_i, + .rst_ni, + .in_i (rf_reg), + .sel_i (raddr_onehot_a), + .out_o (rdata_a_o) + ); + + prim_onehot_mux #( + .Width(DataWidth), + .Inputs(NUM_WORDS) + ) u_rdata_b_mux ( + .clk_i, + .rst_ni, + .in_i (rf_reg), + .sel_i (raddr_onehot_b), + .out_o (rdata_b_o) + ); + end else begin : gen_no_rdata_mux_check + assign rdata_a_o = rf_reg[raddr_a_i]; + assign rdata_b_o = rf_reg[raddr_b_i]; + assign oh_raddr_a_err = 1'b0; + assign oh_raddr_b_err = 1'b0; + end; + + assign err_o = oh_raddr_a_err || oh_raddr_b_err || oh_we_err; // Signal not used in FF register file logic unused_test_en; diff --git a/rtl/ibex_register_file_fpga.sv b/rtl/ibex_register_file_fpga.sv index 420b8fe8bc..0b7c6e0023 100644 --- a/rtl/ibex_register_file_fpga.sv +++ b/rtl/ibex_register_file_fpga.sv @@ -16,6 +16,7 @@ module ibex_register_file_fpga #( parameter int unsigned DataWidth = 32, parameter bit DummyInstructions = 0, parameter bit WrenCheck = 0, + parameter bit RdataMuxCheck = 0, parameter logic [DataWidth-1:0] WordZeroVal = '0 ) ( // Clock and Reset @@ -37,7 +38,7 @@ module ibex_register_file_fpga #( input logic [DataWidth-1:0] wdata_a_i, input logic we_a_i, - // This indicates whether spurious WE are detected. + // This indicates whether spurious WE or one-hot encoded raddr are detected. output logic err_o ); @@ -47,11 +48,109 @@ module ibex_register_file_fpga #( logic [DataWidth-1:0] mem[NUM_WORDS]; logic we; // write enable if writing to any register other than R0 - // async_read a - assign rdata_a_o = (raddr_a_i == '0) ? '0 : mem[raddr_a_i]; + logic [NUM_WORDS-1:0] raddr_onehot_a, raddr_onehot_b; + logic [NUM_WORDS-1:0] raddr_onehot_a_buf, raddr_onehot_b_buf; + logic [DataWidth-1:0] mem_o_a, mem_o_b; - // async_read b - assign rdata_b_o = (raddr_b_i == '0) ? '0 : mem[raddr_b_i]; + // WE strobe and one-hot encoded raddr alert. + logic oh_raddr_a_err, oh_raddr_b_err, oh_we_err; + assign err_o = oh_raddr_a_err || oh_raddr_b_err || oh_we_err; + + if (RdataMuxCheck) begin : gen_rdata_mux_check + // Encode raddr_a/b into one-hot encoded signals. + prim_onehot_enc #( + .OneHotWidth(NUM_WORDS) + ) u_prim_onehot_enc_raddr_a ( + .in_i (raddr_a_i), + .en_i (1'b1), + .out_o (raddr_onehot_a) + ); + + prim_onehot_enc #( + .OneHotWidth(NUM_WORDS) + ) u_prim_onehot_enc_raddr_b ( + .in_i (raddr_b_i), + .en_i (1'b1), + .out_o (raddr_onehot_b) + ); + + // Buffer the one-hot encoded signals so that the checkers + // are not optimized. + prim_buf #( + .Width(NUM_WORDS) + ) u_prim_buf_raddr_a ( + .in_i (raddr_onehot_a), + .out_o(raddr_onehot_a_buf) + ); + + prim_buf #( + .Width(NUM_WORDS) + ) u_prim_buf_raddr_b ( + .in_i (raddr_onehot_b), + .out_o(raddr_onehot_b_buf) + ); + + // Check the one-hot encoded signals for glitches. + prim_onehot_check #( + .AddrWidth(ADDR_WIDTH), + .OneHotWidth(NUM_WORDS), + .AddrCheck(1), + .EnableCheck(0) + ) u_prim_onehot_check_raddr_a ( + .clk_i, + .rst_ni, + .oh_i (raddr_onehot_a_buf), + .addr_i (raddr_a_i), + .en_i (1'b0), + .err_o (oh_raddr_a_err) + ); + + prim_onehot_check #( + .AddrWidth(ADDR_WIDTH), + .OneHotWidth(NUM_WORDS), + .AddrCheck(1), + .EnableCheck(0) + ) u_prim_onehot_check_raddr_b ( + .clk_i, + .rst_ni, + .oh_i (raddr_onehot_b_buf), + .addr_i (raddr_b_i), + .en_i (1'b0), + .err_o (oh_raddr_b_err) + ); + + // MUX register to rdata_a/b_o according to raddr_a/b_onehot. + prim_onehot_mux #( + .Width(DataWidth), + .Inputs(NUM_WORDS) + ) u_rdata_a_mux ( + .clk_i, + .rst_ni, + .in_i (rf_reg), + .sel_i (raddr_onehot_a), + .out_o (mem_o_a) + ); + + prim_onehot_mux #( + .Width(DataWidth), + .Inputs(NUM_WORDS) + ) u_rdata_b_mux ( + .clk_i, + .rst_ni, + .in_i (rf_reg), + .sel_i (raddr_onehot_b), + .out_o (mem_o_b) + ); + + assign rdata_a_o = (raddr_a_i == '0) ? '0 : mem_o_a; + assign rdata_b_o = (raddr_b_i == '0) ? '0 : mem_o_b; + end else begin : gen_no_rdata_mux_check + // async_read a + assign rdata_a_o = (raddr_a_i == '0) ? '0 : mem[raddr_a_i]; + + // async_read b + assign rdata_b_o = (raddr_b_i == '0) ? '0 : mem[raddr_b_i]; + end; // we select assign we = (waddr_a_i == '0) ? 1'b0 : we_a_i; @@ -60,9 +159,9 @@ module ibex_register_file_fpga #( // This checks for spurious WE strobes on the regfile. if (WrenCheck) begin : gen_wren_check // Since the FPGA uses a memory macro, there is only one write-enable strobe to check. - assign err_o = we && !we_a_i; + assign oh_we_err = we && !we_a_i; end else begin : gen_no_wren_check - assign err_o = 1'b0; + assign oh_we_err = 1'b0; end // Note that the SystemVerilog LRM requires variables on the LHS of assignments within diff --git a/rtl/ibex_register_file_latch.sv b/rtl/ibex_register_file_latch.sv index 8b8a8bcc1a..987f46ea30 100644 --- a/rtl/ibex_register_file_latch.sv +++ b/rtl/ibex_register_file_latch.sv @@ -16,6 +16,7 @@ module ibex_register_file_latch #( parameter int unsigned DataWidth = 32, parameter bit DummyInstructions = 0, parameter bit WrenCheck = 0, + parameter bit RdataMuxCheck = 0, parameter logic [DataWidth-1:0] WordZeroVal = '0 ) ( // Clock and Reset @@ -39,7 +40,7 @@ module ibex_register_file_latch #( input logic [DataWidth-1:0] wdata_a_i, input logic we_a_i, - // This indicates whether spurious WE are detected. + // This indicates whether spurious WE or one-hot encoded raddr are detected. output logic err_o ); @@ -50,6 +51,11 @@ module ibex_register_file_latch #( logic [NUM_WORDS-1:0] waddr_onehot_a; + logic [NUM_WORDS-1:0] raddr_onehot_a, raddr_onehot_b; + logic [NUM_WORDS-1:0] raddr_onehot_a_buf, raddr_onehot_b_buf; + + logic oh_raddr_a_err, oh_raddr_b_err, oh_we_err; + logic [NUM_WORDS-1:1] mem_clocks; logic [DataWidth-1:0] wdata_a_q; @@ -62,11 +68,102 @@ module ibex_register_file_latch #( logic clk_int; + assign err_o = oh_raddr_a_err || oh_raddr_b_err || oh_we_err; + ////////// // READ // ////////// - assign rdata_a_o = mem[raddr_a_int]; - assign rdata_b_o = mem[raddr_b_int]; + if (RdataMuxCheck) begin : gen_rdata_mux_check + // Encode raddr_a/b into one-hot encoded signals. + prim_onehot_enc #( + .OneHotWidth(NUM_WORDS) + ) u_prim_onehot_enc_raddr_a ( + .in_i (raddr_a_int), + .en_i (1'b1), + .out_o (raddr_onehot_a) + ); + + prim_onehot_enc #( + .OneHotWidth(NUM_WORDS) + ) u_prim_onehot_enc_raddr_b ( + .in_i (raddr_b_int), + .en_i (1'b1), + .out_o (raddr_onehot_b) + ); + + // Buffer the one-hot encoded signals so that the checkers + // are not optimized. + prim_buf #( + .Width(NUM_WORDS) + ) u_prim_buf_raddr_a ( + .in_i(raddr_onehot_a), + .out_o(raddr_onehot_a_buf) + ); + + prim_buf #( + .Width(NUM_WORDS) + ) u_prim_buf_raddr_b ( + .in_i(raddr_onehot_b), + .out_o(raddr_onehot_b_buf) + ); + + // Check the one-hot encoded signals for glitches. + prim_onehot_check #( + .AddrWidth(ADDR_WIDTH), + .OneHotWidth(NUM_WORDS), + .AddrCheck(1), + .EnableCheck(0) + ) u_prim_onehot_check_raddr_a ( + .clk_i, + .rst_ni, + .oh_i (raddr_onehot_a_buf), + .addr_i (raddr_b_int), + .en_i (1'b0), + .err_o (oh_raddr_a_err) + ); + + prim_onehot_check #( + .AddrWidth(ADDR_WIDTH), + .OneHotWidth(NUM_WORDS), + .AddrCheck(1), + .EnableCheck(0) + ) u_prim_onehot_check_raddr_b ( + .clk_i, + .rst_ni, + .oh_i (raddr_onehot_b_buf), + .addr_i (raddr_b_int), + .en_i (1'b0), + .err_o (oh_raddr_b_err) + ); + + // MUX register to rdata_a/b_o according to raddr_a/b_onehot. + prim_onehot_mux #( + .Width(DataWidth), + .Inputs(NUM_WORDS) + ) u_rdata_a_mux ( + .clk_i, + .rst_ni, + .in_i (mem), + .sel_i (raddr_onehot_a), + .out_o (rdata_a_o) + ); + + prim_onehot_mux #( + .Width(DataWidth), + .Inputs(NUM_WORDS) + ) u_rdata_b_mux ( + .clk_i, + .rst_ni, + .in_i (mem), + .sel_i (raddr_onehot_b), + .out_o (rdata_b_o) + ); + end else begin : gen_no_rdata_mux_check + assign rdata_a_o = mem[raddr_a_int]; + assign rdata_b_o = mem[raddr_b_int]; + assign oh_raddr_a_err = 1'b0; + assign oh_raddr_b_err = 1'b0; + end; /////////// // WRITE // @@ -125,12 +222,12 @@ module ibex_register_file_latch #( .oh_i(waddr_onehot_a_buf), .addr_i(waddr_a_i), .en_i(we_a_i), - .err_o + .err_o(oh_we_err) ); end else begin : gen_no_wren_check logic unused_strobe; assign unused_strobe = waddr_onehot_a[0]; // this is never read from in this case - assign err_o = 1'b0; + assign oh_we_err = 1'b0; end // Individual clock gating (if integrated clock-gating cells are available) diff --git a/rtl/ibex_top.sv b/rtl/ibex_top.sv index f4e97e8b6a..dff77b0cde 100644 --- a/rtl/ibex_top.sv +++ b/rtl/ibex_top.sv @@ -140,14 +140,15 @@ module ibex_top import ibex_pkg::*; #( input logic scan_rst_ni ); - localparam bit Lockstep = SecureIbex; - localparam bit ResetAll = Lockstep; - localparam bit DummyInstructions = SecureIbex; - localparam bit RegFileECC = SecureIbex; - localparam bit RegFileWrenCheck = SecureIbex; - localparam int unsigned RegFileDataWidth = RegFileECC ? 32 + 7 : 32; - localparam bit MemECC = SecureIbex; - localparam int unsigned MemDataWidth = MemECC ? 32 + 7 : 32; + localparam bit Lockstep = SecureIbex; + localparam bit ResetAll = Lockstep; + localparam bit DummyInstructions = SecureIbex; + localparam bit RegFileECC = SecureIbex; + localparam bit RegFileWrenCheck = SecureIbex; + localparam bit RegFileRdataMuxCheck = SecureIbex; + localparam int unsigned RegFileDataWidth = RegFileECC ? 32 + 7 : 32; + localparam bit MemECC = SecureIbex; + localparam int unsigned MemDataWidth = MemECC ? 32 + 7 : 32; // Icache parameters localparam int unsigned BusSizeECC = ICacheECC ? (BUS_SIZE + 7) : BUS_SIZE; localparam int unsigned LineSizeECC = BusSizeECC * IC_LINE_BEATS; @@ -421,6 +422,7 @@ module ibex_top import ibex_pkg::*; #( .DummyInstructions(DummyInstructions), // SEC_CM: DATA_REG_SW.GLITCH_DETECT .WrenCheck (RegFileWrenCheck), + .RdataMuxCheck (RegFileRdataMuxCheck), .WordZeroVal (RegFileDataWidth'(prim_secded_pkg::SecdedInv3932ZeroWord)) ) register_file_i ( .clk_i (clk), @@ -446,6 +448,7 @@ module ibex_top import ibex_pkg::*; #( .DummyInstructions(DummyInstructions), // SEC_CM: DATA_REG_SW.GLITCH_DETECT .WrenCheck (RegFileWrenCheck), + .RdataMuxCheck (RegFileRdataMuxCheck), .WordZeroVal (RegFileDataWidth'(prim_secded_pkg::SecdedInv3932ZeroWord)) ) register_file_i ( .clk_i (clk), @@ -471,6 +474,7 @@ module ibex_top import ibex_pkg::*; #( .DummyInstructions(DummyInstructions), // SEC_CM: DATA_REG_SW.GLITCH_DETECT .WrenCheck (RegFileWrenCheck), + .RdataMuxCheck (RegFileRdataMuxCheck), .WordZeroVal (RegFileDataWidth'(prim_secded_pkg::SecdedInv3932ZeroWord)) ) register_file_i ( .clk_i (clk),