Skip to content

Commit

Permalink
[rtl] Guard against false memory responses for secure configurations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
GregAC committed May 16, 2024
1 parent eea2bf0 commit 2e34f95
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 7 deletions.
42 changes: 37 additions & 5 deletions rtl/ibex_core.sv
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),

Expand All @@ -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),
Expand Down Expand Up @@ -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 //
/////////////////////////////
Expand Down Expand Up @@ -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 //
//////////
Expand Down
18 changes: 18 additions & 0 deletions rtl/ibex_ex_block.sv
Original file line number Diff line number Diff line change
Expand Up @@ -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

Check warning on line 208 in rtl/ibex_ex_block.sv

View workflow job for this annotation

GitHub Actions / verible-lint

[verible-verilog-lint] reported by reviewdog 🐶 All generate block statements must have a label [Style: generate-statements] [generate-label] Raw Output: message:"All generate block statements must have a label [Style: generate-statements] [generate-label]" location:{path:"./rtl/ibex_ex_block.sv" range:{start:{line:208 column:12}}} severity:WARNING source:{name:"verible-verilog-lint" url:"https://github.com/chipsalliance/verible"}
assign sva_multdiv_fsm_idle = 1'b1;
end

// Mark the sva_multduv_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
19 changes: 19 additions & 0 deletions rtl/ibex_id_stage.sv
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
23 changes: 23 additions & 0 deletions rtl/ibex_multdiv_fast.sv
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
12 changes: 12 additions & 0 deletions rtl/ibex_multdiv_slow.sv
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ module ibex_multdiv_slow
end // (mult_sel_i || div_sel_i)
end


//////////////////////////////////////////
// Mutliplier / Divider state registers //
//////////////////////////////////////////
Expand Down Expand Up @@ -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"
Expand Down
3 changes: 1 addition & 2 deletions rtl/ibex_wb_stage.sv
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 2e34f95

Please sign in to comment.