From 8fe62eaba9625fc2332265fcb770bcf00ce6f6ba Mon Sep 17 00:00:00 2001 From: Greg Chadwick Date: Thu, 7 Mar 2024 14:01:20 +0000 Subject: [PATCH] [rtl] Guard against false memory responses for secure configurations With this change all memory responses are only acted on if Ibex is expecting them for all secure configurations. Previously an error response that was injected onto the bus would trigger an exception that shouldn't occur (in particular breaking the functioning of the multiply state machine). In addition for configurations without the writeback stage an injected load data response could trigger an incorrect write to the register file. This is only applied to the secure configurations, non-secure configurations assume correct adherence to the bus protocol meaning a response will only be seen if a request is outstanding. --- rtl/ibex_core.sv | 42 +++++++++++++++++++++++++++++++++++----- rtl/ibex_ex_block.sv | 18 +++++++++++++++++ rtl/ibex_id_stage.sv | 19 ++++++++++++++++++ rtl/ibex_multdiv_fast.sv | 23 ++++++++++++++++++++++ rtl/ibex_multdiv_slow.sv | 12 ++++++++++++ rtl/ibex_wb_stage.sv | 3 +-- 6 files changed, 110 insertions(+), 7 deletions(-) diff --git a/rtl/ibex_core.sv b/rtl/ibex_core.sv index a175efbb94..e0a8c6d2a3 100644 --- a/rtl/ibex_core.sv +++ b/rtl/ibex_core.sv @@ -211,11 +211,14 @@ module ibex_core import ibex_pkg::*; #( exc_cause_t exc_cause; // Exception cause logic instr_intg_err; - logic lsu_load_err; - logic lsu_store_err; + logic lsu_load_err, lsu_load_err_raw; + logic lsu_store_err, lsu_store_err_raw; logic lsu_load_resp_intg_err; logic lsu_store_resp_intg_err; + logic expecting_load_resp_id; + logic expecting_store_resp_id; + // LSU signals logic lsu_addr_incr_req; logic [31:0] lsu_addr_last; @@ -290,6 +293,7 @@ module ibex_core import ibex_pkg::*; #( logic [1:0] lsu_type; logic lsu_sign_ext; logic lsu_req; + logic lsu_rdata_valid; logic [31:0] lsu_wdata; logic lsu_req_done; @@ -637,6 +641,9 @@ module ibex_core import ibex_pkg::*; #( .lsu_store_err_i (lsu_store_err), .lsu_store_resp_intg_err_i(lsu_store_resp_intg_err), + .expecting_load_resp_o (expecting_load_resp_id), + .expecting_store_resp_o(expecting_store_resp_id), + // Interrupt Signals .csr_mstatus_mie_i(csr_mstatus_mie), .irq_pending_i (irq_pending_o), @@ -774,7 +781,7 @@ module ibex_core import ibex_pkg::*; #( .lsu_sign_ext_i(lsu_sign_ext), .lsu_rdata_o (rf_wdata_lsu), - .lsu_rdata_valid_o(rf_we_lsu), + .lsu_rdata_valid_o(lsu_rdata_valid), .lsu_req_i (lsu_req), .lsu_req_done_o (lsu_req_done), @@ -787,9 +794,9 @@ module ibex_core import ibex_pkg::*; #( .lsu_resp_valid_o(lsu_resp_valid), // exception signals - .load_err_o (lsu_load_err), + .load_err_o (lsu_load_err_raw), .load_resp_intg_err_o (lsu_load_resp_intg_err), - .store_err_o (lsu_store_err), + .store_err_o (lsu_store_err_raw), .store_resp_intg_err_o(lsu_store_resp_intg_err), .busy_o(lsu_busy), @@ -844,6 +851,28 @@ module ibex_core import ibex_pkg::*; #( .instr_done_wb_o(instr_done_wb) ); + if (SecureIbex) begin : g_check_mem_response + // For secure configurations only process load/store responses if we're expecting them to guard + // against false responses being injected on to the bus + assign lsu_load_err = lsu_load_err_raw & (outstanding_load_wb | expecting_load_resp_id); + assign lsu_store_err = lsu_store_err_raw & (outstanding_store_wb | expecting_store_resp_id); + assign rf_we_lsu = lsu_rdata_valid & (outstanding_load_wb | expecting_load_resp_id); + end else begin : g_no_check_mem_response + // For non-secure configurations trust the bus protocol is being followed and we'll only ever + // see a response if we have an outstanding request. + assign lsu_load_err = lsu_load_err_raw; + assign lsu_store_err = lsu_store_err_raw; + assign rf_we_lsu = lsu_rdata_valid; + + // expected_load_resp_id/expected_store_resp_id signals are only used to guard against false + // responses so they are unused in non-secure configurations + logic unused_expecting_load_resp_id; + logic unused_expecting_store_resp_id; + + assign unused_expecting_load_resp_id = expecting_load_resp_id; + assign unused_expecting_store_resp_id = expecting_store_resp_id; + end + ///////////////////////////// // Register file interface // ///////////////////////////// @@ -1810,6 +1839,9 @@ module ibex_core import ibex_pkg::*; #( // Certain parameter combinations are not supported `ASSERT_INIT(IllegalParamSecure, !(SecureIbex && (RV32M == RV32MNone))) + // If the ID stage signals its ready the mult/div FSMs must be idle in the following cycle + `ASSERT(MultDivFSMIdleOnIdReady, id_in_ready |=> ex_block_i.sva_multdiv_fsm_idle) + ////////// // FCOV // ////////// diff --git a/rtl/ibex_ex_block.sv b/rtl/ibex_ex_block.sv index ee900164b9..c44a931691 100644 --- a/rtl/ibex_ex_block.sv +++ b/rtl/ibex_ex_block.sv @@ -196,4 +196,22 @@ module ibex_ex_block #( // final cycle of ALU operation). assign ex_valid_o = multdiv_sel ? multdiv_valid : ~(|alu_imd_val_we); +`ifdef INC_ASSERT + // This is intended to be accessed via hierarchal references so isn't output from this module nor + // used in any logic in this module + logic sva_multdiv_fsm_idle; + + if (RV32M == RV32MSlow) begin : gen_multdiv_sva_idle_slow + assign sva_multdiv_fsm_idle = gen_multdiv_slow.multdiv_i.sva_fsm_idle; + end else if (RV32M == RV32MFast || RV32M == RV32MSingleCycle) begin : gen_multdiv_sva_idle_fast + assign sva_multdiv_fsm_idle = gen_multdiv_fast.multdiv_i.sva_fsm_idle; + end else begin : gen_multdiv_sva_idle_none + assign sva_multdiv_fsm_idle = 1'b1; + end + + // Mark the sva_multdiv_fsm_idle as unused to avoid lint issues + logic unused_sva_multdiv_fsm_idle; + assign unused_sva_multdiv_fsm_idle = sva_multdiv_fsm_idle; +`endif + endmodule diff --git a/rtl/ibex_id_stage.sv b/rtl/ibex_id_stage.sv index 95b4fa9b97..3a358af7ab 100644 --- a/rtl/ibex_id_stage.sv +++ b/rtl/ibex_id_stage.sv @@ -135,6 +135,9 @@ module ibex_id_stage #( input logic lsu_store_err_i, input logic lsu_store_resp_intg_err_i, + output logic expecting_load_resp_o, + output logic expecting_store_resp_o, + // Debug Signal output logic debug_mode_o, output logic debug_mode_entering_o, @@ -1016,6 +1019,11 @@ module ibex_id_stage #( assign perf_dside_wait_o = instr_valid_i & ~instr_kill & (outstanding_memory_access | stall_ld_hz); + + // With writeback stage load/store responses are processed in the writeback stage so the ID/EX + // stage is never expecting a load or store response. + assign expecting_load_resp_o = 1'b0; + assign expecting_store_resp_o = 1'b0; end else begin : gen_no_stall_mem assign multicycle_done = lsu_req_dec ? lsu_resp_valid_i : ex_valid_i; @@ -1044,6 +1052,13 @@ module ibex_id_stage #( assign rf_rd_a_wb_match_o = 1'b0; assign rf_rd_b_wb_match_o = 1'b0; + // First cycle of a load or store is always the request. We're expecting a response the cycles + // following. Note if the request isn't immediatly accepted these signals will still assert. + // However in this case the LSU won't signal a response as it's still waiting for the grant + // (even if the external memory bus signals are glitched to generate a false response). + assign expecting_load_resp_o = instr_valid_i & lsu_req_dec & ~instr_first_cycle & ~lsu_we; + assign expecting_store_resp_o = instr_valid_i & lsu_req_dec & ~instr_first_cycle & lsu_we; + // Unused Writeback stage only IO & wiring // Assign inputs and internal wiring to unused signals to satisfy lint checks // Tie-off outputs to constant values @@ -1143,6 +1158,10 @@ module ibex_id_stage #( // includes Xs `ASSERT(IbexDuplicateInstrMatch, instr_valid_i |-> instr_rdata_i === instr_rdata_alu_i) + // Check that when ID stage is ready for next instruction FSM is in FIRST_CYCLE state the + // following cycle (when the new instructon may begin executing). + `ASSERT(IbexMoveToFirstCycleWhenIdReady, id_in_ready_o |=> id_fsm_q == FIRST_CYCLE) + `ifdef CHECK_MISALIGNED `ASSERT(IbexMisalignedMemoryAccess, !lsu_addr_incr_req_i) `endif diff --git a/rtl/ibex_multdiv_fast.sv b/rtl/ibex_multdiv_fast.sv index 3f0d27ac3b..2e48dc5270 100644 --- a/rtl/ibex_multdiv_fast.sv +++ b/rtl/ibex_multdiv_fast.sv @@ -84,6 +84,9 @@ module ibex_multdiv_fast #( logic mult_en_internal; logic div_en_internal; + // Used for SVA purposes, no functional relevance + logic sva_mul_fsm_idle; + typedef enum logic [2:0] { MD_IDLE, MD_ABS_A, MD_ABS_B, MD_COMP, MD_LAST, MD_CHANGE_SIGN, MD_FINISH } md_fsm_e; @@ -254,6 +257,8 @@ module ibex_multdiv_fast #( // States must be knwon/valid. `ASSERT_KNOWN(IbexMultStateKnown, mult_state_q) + assign sva_mul_fsm_idle = mult_state_q == MULL; + // The fast multiplier uses one 17 bit multiplier to compute MUL instructions in 3 cycles // and MULH instructions in 4 cycles. end else begin : gen_mult_fast @@ -372,6 +377,8 @@ module ibex_multdiv_fast #( // States must be knwon/valid. `ASSERT_KNOWN(IbexMultStateKnown, mult_state_q) + assign sva_mul_fsm_idle = mult_state_q == ALBL; + end // gen_mult_fast // Divider @@ -518,12 +525,28 @@ module ibex_multdiv_fast #( endcase // md_state_q end + assign valid_o = mult_valid | div_valid; // States must be knwon/valid. `ASSERT(IbexMultDivStateValid, md_state_q inside { MD_IDLE, MD_ABS_A, MD_ABS_B, MD_COMP, MD_LAST, MD_CHANGE_SIGN, MD_FINISH}) +`ifdef INC_ASSERT + logic sva_fsm_idle; + logic unused_sva_fsm_idle; + + // This is intended to be accessed via hierarchal references so isn't output from this module nor + // used in any logic in this module + assign sva_fsm_idle = (md_state_q == MD_IDLE) && sva_mul_fsm_idle; + // Mark the sva_fsm_idle as unused to avoid lint issues + assign unused_sva_fsm_idle = sva_fsm_idle; +`else + logic unused_sva_mul_fsm_idle; + + assign unused_sva_mul_fsm_idle = sva_mul_fsm_idle; +`endif + `ifdef FORMAL `ifdef YOSYS `include "formal_tb_frag.svh" diff --git a/rtl/ibex_multdiv_slow.sv b/rtl/ibex_multdiv_slow.sv index 214d3f599e..c409e2965f 100644 --- a/rtl/ibex_multdiv_slow.sv +++ b/rtl/ibex_multdiv_slow.sv @@ -327,6 +327,7 @@ module ibex_multdiv_slow end // (mult_sel_i || div_sel_i) end + ////////////////////////////////////////// // Mutliplier / Divider state registers // ////////////////////////////////////////// @@ -369,6 +370,17 @@ module ibex_multdiv_slow MD_IDLE, MD_ABS_A, MD_ABS_B, MD_COMP, MD_LAST, MD_CHANGE_SIGN, MD_FINISH }, clk_i, !rst_ni) +`ifdef INC_ASSERT + logic sva_fsm_idle; + logic unused_sva_fsm_idle; + + // This is intended to be accessed via hierarchal references so isn't output from this module nor + // used in any logic in this module + assign sva_fsm_idle = (md_state_q == MD_IDLE); + // Mark the sva_fsm_idle as unused to avoid lint issues + assign unused_sva_fsm_idle = sva_fsm_idle; +`endif + `ifdef FORMAL `ifdef YOSYS `include "formal_tb_frag.svh" diff --git a/rtl/ibex_wb_stage.sv b/rtl/ibex_wb_stage.sv index 5352848955..38ce421ca0 100644 --- a/rtl/ibex_wb_stage.sv +++ b/rtl/ibex_wb_stage.sv @@ -166,8 +166,7 @@ module ibex_wb_stage #( // that returns too late to be used on the forwarding path. assign rf_wdata_fwd_wb_o = rf_wdata_wb_q; - // For FI hardening, only forward LSU write enable if we're actually waiting for it. - assign rf_wdata_wb_mux_we[1] = outstanding_load_wb_o & rf_we_lsu_i; + assign rf_wdata_wb_mux_we[1] = rf_we_lsu_i; if (DummyInstructions) begin : g_dummy_instr_wb logic dummy_instr_wb_q;