-
Notifications
You must be signed in to change notification settings - Fork 554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[rtl] Fix FI vulnerability in RF #2117
Conversation
457d502
to
6582167
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks @nasahlpa for the PR! The implementation looks good to me and I believe the area overhead is more than acceptable. Thanks for running all the checks and analysis (and for also taking care of aligning the latch-based and the FPGA reg files even though we're not using them for OT), this is great work!
In my view, we should merge this but we first need to discuss this in our Hardware Sync-Up and @GregAC needs to be looped in as well.
Once we have found agreement, we should also update the documentation to list the one hot check inside the register file, see e.g. https://ibex-core.readthedocs.io/en/latest/03_reference/security.html?highlight=security#register-file-write-enable-glitch-detection
rtl/ibex_register_file_ff.sv
Outdated
.rst_ni, | ||
.oh_i (raddr_onehot_a_buf), | ||
.addr_i (raddr_a_i), | ||
.en_i (1'b0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe want to add a comment that the en_i
input is unused as the EnableCheck
parameter is disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. I actually had to set en_i = 1'b1
as EnableCheck
needs to be on when having AddrCheck=1
.
The reason for this is the following condition:
`ASSERT_INIT(AddrImpliesEnable_A, AddrCheck && EnableCheck || !AddrCheck) |
I've added comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the comments.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, it is sufficient to do the one-hot encoding in here (not covered by the lockstep) because the same gates driving the raddr_a/b_i
signals used here also drive the wires ultimately going into the lockstep comparison. I.e., if someone managed to fault raddr_a_i
here, this would still be caught by the lockstep countermeasure, right?
If yes, this would be perfect because it means we don't have to touch the lockstep interfacing and checking logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
In ibex_top.sv
the rf_raddr_a/b
signals are generated by the Ibex core:
Lines 336 to 337 in 38da151
.rf_raddr_a_o (rf_raddr_a), | |
.rf_raddr_b_o (rf_raddr_b), |
forwarded to the RF:
Lines 433 to 435 in 38da151
.raddr_a_i(rf_raddr_a), | |
.rdata_a_o(rf_rdata_a_ecc), | |
.raddr_b_i(rf_raddr_b), |
and buffered for the lockstep:
Lines 862 to 863 in 38da151
rf_raddr_a, | |
rf_raddr_b, |
Inside the lockstep, again the rf_raddr_a/b
signals are generated by the shadow Ibex core:
Lines 374 to 375 in 38da151
.rf_raddr_a_o (shadow_outputs_d.rf_raddr_a), | |
.rf_raddr_b_o (shadow_outputs_d.rf_raddr_b), |
And finally the comparison happens. Here, the buffered rf_raddr_a/b
signals from the main core are then compared to the signals generated by the shadow core.
If a bit-flip inside the RF manipulates the rf_raddr_a/b
signals, these faulty signals will get buffered for the lockstep:
Lines 862 to 863 in 38da151
rf_raddr_a, | |
rf_raddr_b, |
And the fault is detected when comparing the main vs shadow rf_raddr_a/b
signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the analysis and writing it up here. LGTM!
Thanks for your valuable feedback, @vogelpi! I've addressed all of your comments and CI is also passing. |
b77d5e0
to
43229d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to me, nice catch @nasahlpa couple of minor comments then should be good to go.
doc/03_reference/security.rst
Outdated
Register file read addresses glitch detection | ||
------------------------------------------- | ||
|
||
When Ibex is configured with the SecureIbex parameter, the read addresses provided to the register file are converted to one-hot encoded signals and these signals are checked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may want to expand slightly on what the checks are.
rtl/ibex_register_file_ff.sv
Outdated
// When AddrCheck=1 also EnableCheck needs to be 1. | ||
.EnableCheck(1), | ||
// Avoid that raddr=0 triggers an error. | ||
.StrictCheck(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what scenario does this happen? When the incoming address is 0 we'll get a one-hot encoded signal with the bottom bit set and the onehot encoders are always enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, you are right! I've removed StrictCheck=0
.
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 <nasahlpa@lowrisc.org>
Full DV regression run is now completed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nasahlpa, this is a nice security improvement!
Thanks to all of you for the review! Merging it now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for addressing the remaining comments 👍
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.