From 457d502e3d2574d703b2e883c9fad8941419ea33 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 --- dv/uvm/core_ibex/common/prim/prim_and2.sv | 27 ++++++ dv/uvm/core_ibex/ibex_dv.f | 5 + ibex_top.core | 2 + rtl/ibex_register_file_ff.sv | 106 +++++++++++++++++++- rtl/ibex_register_file_fpga.sv | 113 ++++++++++++++++++++-- rtl/ibex_register_file_latch.sv | 106 +++++++++++++++++++- rtl/ibex_top.sv | 20 ++-- 7 files changed, 354 insertions(+), 25 deletions(-) create mode 100644 dv/uvm/core_ibex/common/prim/prim_and2.sv diff --git a/dv/uvm/core_ibex/common/prim/prim_and2.sv b/dv/uvm/core_ibex/common/prim/prim_and2.sv new file mode 100644 index 0000000000..157d4dba67 --- /dev/null +++ b/dv/uvm/core_ibex/common/prim/prim_and2.sv @@ -0,0 +1,27 @@ +// Copyright lowRISC contributors. +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +// Abstract primitives wrapper. +// +// This file is a stop-gap until the DV file list is generated by FuseSoC. +// Its contents are taken from the file which would be generated by FuseSoC. +// https://github.com/lowRISC/ibex/issues/893 + +module prim_and2 #( + parameter int Width = 1 +) ( + input [Width-1:0] in0_i, + input [Width-1:0] in1_i, + output logic [Width-1:0] out_o +); + +if (1) begin : gen_generic + prim_generic_and2 #( + .Width(Width) + ) u_impl_generic ( + .* + ); +end + +endmodule diff --git a/dv/uvm/core_ibex/ibex_dv.f b/dv/uvm/core_ibex/ibex_dv.f index fa79191b26..cbc5b9b80e 100644 --- a/dv/uvm/core_ibex/ibex_dv.f +++ b/dv/uvm/core_ibex/ibex_dv.f @@ -45,6 +45,8 @@ ${PRJ_DIR}/dv/uvm/core_ibex/common/prim/prim_clock_mux2.sv ${LOWRISC_IP_DIR}/ip/prim_generic/rtl/prim_generic_flop.sv ${PRJ_DIR}/dv/uvm/core_ibex/common/prim/prim_flop.sv +${LOWRISC_IP_DIR}/ip/prim_generic/rtl/prim_generic_and2.sv +${PRJ_DIR}/dv/uvm/core_ibex/common/prim/prim_generic_and2.sv // Shared lowRISC code ${LOWRISC_IP_DIR}/ip/prim/rtl/prim_cipher_pkg.sv @@ -64,6 +66,9 @@ ${LOWRISC_IP_DIR}/ip/prim/rtl/prim_secded_72_64_enc.sv ${LOWRISC_IP_DIR}/ip/prim/rtl/prim_secded_72_64_dec.sv ${LOWRISC_IP_DIR}/ip/prim/rtl/prim_onehot_check.sv +${LOWRISC_IP_DIR}/ip/prim/rtl/prim_onehot_check.sv +${LOWRISC_IP_DIR}/ip/prim/rtl/prim_onehot_enc.sv +${LOWRISC_IP_DIR}/ip/prim/rtl/prim_onehot_mux.sv ${LOWRISC_IP_DIR}/ip/prim/rtl/prim_mubi_pkg.sv // ibex CORE RTL files diff --git a/ibex_top.core b/ibex_top.core index db33a72f84..e8e5d011e9 100644 --- a/ibex_top.core +++ b/ibex_top.core @@ -10,11 +10,13 @@ filesets: depend: - lowrisc:ibex:ibex_pkg - lowrisc:ibex:ibex_core + - lowrisc:prim:and2 - lowrisc:prim:buf - lowrisc:prim:clock_mux2 - lowrisc:prim:flop - lowrisc:prim:ram_1p_scr - lowrisc:prim:onehot_check + - lowrisc:prim:onehot files: - rtl/ibex_register_file_ff.sv # generic FF-based - rtl/ibex_register_file_fpga.sv # FPGA diff --git a/rtl/ibex_register_file_ff.sv b/rtl/ibex_register_file_ff.sv index 45890db886..80e25e3a5f 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,8 @@ module ibex_register_file_ff #( logic [DataWidth-1:0] rf_reg [NUM_WORDS]; logic [NUM_WORDS-1:0] we_a_dec; + 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 +81,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 +133,101 @@ 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. + logic [NUM_WORDS-1:0] raddr_onehot_a, raddr_onehot_b; + logic [NUM_WORDS-1:0] raddr_onehot_a_buf, raddr_onehot_b_buf; + 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..eb2af211ef 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 [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. + logic [NUM_WORDS-1:0] raddr_onehot_a, raddr_onehot_b; + logic [NUM_WORDS-1:0] raddr_onehot_a_buf, raddr_onehot_b_buf; + 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 (mem), + .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 (mem), + .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..76999d6d3e 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,8 @@ module ibex_register_file_latch #( logic [NUM_WORDS-1:0] waddr_onehot_a; + 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 +65,104 @@ 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. + logic [NUM_WORDS-1:0] raddr_onehot_a, raddr_onehot_b; + logic [NUM_WORDS-1:0] raddr_onehot_a_buf, raddr_onehot_b_buf; + 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 +221,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),