[rtl] Harden lockstep enable against FI #2129

nasahlpa · 2024-01-17T12:14:55Z

Currently, the dual-core lockstep FI mitigation is enabled/disabled using a single bit. For transient bit-flips, this is not problematic, as one bit-flip into this signal and one bit into the Ibex is required to threaten the security of the system.

However, a permanent stuck-at-0 fault could disable the lockstep completely by targeting this signal. Then, only a single, additional fault (transient or permanent) is required.

This PR enhances the FI resilience of the Ibex lockstep by encoding this single bit into a ibex_mubi_t signal, i.e., a 4-bit multi-bit signal. Note that the MUXes:

ibex/rtl/ibex_lockstep.sv

Line 138 in 2fbf046

(rst_shadow_set_d == IbexMuBiOn) ? rst_shadow_cnt_q : rst_shadow_cnt_incr;

and

ibex/rtl/ibex_lockstep.sv

Line 468 in 2fbf046

(enable_cmp_q == IbexMuBiOn) & (shadow_outputs_q != core_outputs_q[0]);

are not redundant. However, as the goal is to protect the previously 1-bit select signal against stuck-at-0 faults, increasing the bit-width to 4-bit significantly increases security against this threat.

I opted for going with a ibex_mubi_t instead of a mubi4/8 as it seems that only ibex_mubi_t is frequently used in the core.

Tracked in #20779.

Full DV regression looks good.

nasahlpa · 2024-01-18T10:29:28Z

rtl/ibex_lockstep.sv

@@ -157,7 +163,7 @@ module ibex_lockstep import ibex_pkg::*; #(
  prim_clock_mux2 #(
    .NoFpgaBufG(1'b1)
  ) u_prim_rst_shadow_n_mux2 (
-    .clk0_i(rst_shadow_set_q),
+    .clk0_i(rst_shadow_set_single_bit),


This is needed, because when doing this:

Suggested change

.clk0_i(rst_shadow_set_single_bit),

.clk0_i(rst_shadow_set_q[0]),

I get the IMPERFECTSCH linting error.

Does the IMPERFECTSCH warning cause CI to fail? If not, I'd be inclined to use the rst_shadow_set_q[0] form. I notice that the warning was removed from Verilator with version 5 (https://verilator.org/guide/latest/warnings.html).

Yes, CI fails because of this IMPERFECTSCH warning.

I would also prefer using rst_shadow_set_q[0], but I couldn't find a way of getting rid of this warning in CI.

A solution: upgrade our Verilator dependency :-)

I think we can probably just do that. Would you mind filing an issue to remind us? ("Upgrade Verilator, then remove hack I had to do in ibex_lockstep.sv" or something similar)

Good idea, tracked in issue #2131.

andreaskurth

Thanks for catching this and working on a fix, @nasahlpa! I agree we should make changes to harden lockstep enable against FI, but I'm not sure the PR as-is suffices; please see my comments

andreaskurth · 2024-01-18T10:25:43Z

rtl/ibex_lockstep.sv

-  assign outputs_mismatch       = enable_cmp_q & (shadow_outputs_q != core_outputs_q[0]);
+  assign outputs_mismatch =
+    (enable_cmp_q == IbexMuBiOn) & (shadow_outputs_q != core_outputs_q[0]);


Wouldn't single-bit FI on enable_cmp_q still be sufficient to evade this comparison? Put differently, shouldn't we use enable_cmp_q != IbexMuBiOff as condition here?

Yes, you are right. I've changed this.

andreaskurth · 2024-01-18T10:30:59Z

rtl/ibex_lockstep.sv


  always_ff @(posedge clk_i or negedge rst_ni) begin
    if (!rst_ni) begin
      rst_shadow_cnt_q <= '0;
-      enable_cmp_q     <= '0;
+      enable_cmp_q     <= IbexMuBiOff;
    end else begin
      rst_shadow_cnt_q <= rst_shadow_cnt_d;
      enable_cmp_q     <= rst_shadow_set_q;


enable_cmp_q may also benefit from being implemented with a prim_flop to avoid optimization, right?

I've implemented enable_cmp with a prim_flop to avoid optimization. Thanks!

andreaskurth · 2024-01-18T10:34:02Z

rtl/ibex_lockstep.sv

+  assign rst_shadow_set_d =
+    (rst_shadow_cnt_q == LockstepOffsetW'(LockstepOffset - 1)) ? IbexMuBiOn : IbexMuBiOff;


Here a MuBi is computed from a strict bit-by-bit equality, so wouldn't a single-bit FI on rst_shadow_cnt_q be sufficient to control the MuBi?

I think this could be mitigated by using a hardened counter for rst_shadow_cnt

I also recognized that the counter here could be a weak point. However, does using prim_count actually mitigate this problem? I was thinking of replacing the counter with the following:

prim_count #( .Width(LockstepOffsetW) ) u_rst_shadow_cnt ( .clk_i(clk_i), .rst_ni(rst_ni), .clr_i('0), .set_i('0), Done .set_cnt_i('0), Done .incr_en_i(rst_shadow_set_d == IbexMuBiOn), .decr_en_i(1'b0), Done .step_i(LockstepOffsetW'(1'b1)), Done .cnt_o(rst_shadow_cnt_q), .cnt_next_o(), Done .err_o(cnt_err) );

However, if a stuck-at-0 fault inside the prim_count permanently sets the incr_en signal to 0, we would have the same issue again?

prim_count saturates and doesn't overflow, so how about setting incr_en_i as well as step_i to 1, and then use >= for comparing cnt_o to derive the MuBi value? Surely stuck-at-0 on step_i or incr_en_i would still bypass this countermeasure, but these would be tied to logic rails, which AFAIK is much more difficult to attack than bits stored in FFs/latches.

I agree with @andreaskurth .

I am not sure if we should also check whether rst_shadow_set_q contains a valid value and raise an alert if not. We do this in other places as well. However, if rst_shadow_set_q was invalid, also enable_cmp_q would become invalid eventually and the comparison was started: whenever it's not off, the comparison is enabled. So it's probably fine. We may want to document this though to prevent someone from changing this accidentally.

vogelpi

Thanks for the PR @nasahlpa , this looks good. Adding a prim_count sounds like a good idea to me.

rtl/ibex_pkg.sv

vogelpi · 2024-01-18T22:39:19Z

rtl/ibex_lockstep.sv

+  assign rst_shadow_set_d =
+    (rst_shadow_cnt_q == LockstepOffsetW'(LockstepOffset - 1)) ? IbexMuBiOn : IbexMuBiOff;


I agree with @andreaskurth .

I am not sure if we should also check whether rst_shadow_set_q contains a valid value and raise an alert if not. We do this in other places as well. However, if rst_shadow_set_q was invalid, also enable_cmp_q would become invalid eventually and the comparison was started: whenever it's not off, the comparison is enabled. So it's probably fine. We may want to document this though to prevent someone from changing this accidentally.

rtl/ibex_lockstep.sv

To be consistent between projects, this PR updates the Verilator version to 4.210, which is also used by OpenTitan. The reason for this change is that in lowRISC#2129 Verilator linting issues occured that did not occur in OpenTitan. Closes lowRISC#2131. Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>

rtl/ibex_lockstep.sv

To be consistent between projects, this PR updates the Verilator version to 4.210, which is also used by OpenTitan. The reason for this change is that in #2129 Verilator linting issues occured that did not occur in OpenTitan. Closes #2131. Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>

nasahlpa · 2024-01-20T17:08:57Z

Thanks for the PR @nasahlpa , this looks good. Adding a prim_count sounds like a good idea to me.

Thanks for all the helpful suggestions! I've updated the code and this PR is ready for a second round of reviews :-)

rswarbrick

A couple of last nitty questions, but this looks really good to me.

rswarbrick · 2024-01-20T18:01:05Z

dv/uvm/core_ibex/tb/core_ibex_tb_top.sv

@@ -331,6 +331,12 @@ module core_ibex_tb_top;
    run_test();
  end

+  // Disable the assertion for prim_count in case lockstep (set by SecureIbex) is enabled.


Hmm, this "assert connected" flag is an ugly feature of the design of the prim. Yuck!

But can we get it wired up without hacking things by using something like the ASSERT_ERROR_TRIGGER_ALERT macro? I suspect not, because that macro seems to have an opinion about how alerts should be signaled (true in OpenTitan; not true for Ibex, I think).

If that doesn't work, could we add a bit of explanation in this comment about what we're working around?

rswarbrick · 2024-01-20T18:03:41Z

rtl/ibex_lockstep.sv


-  assign rst_shadow_cnt_incr = rst_shadow_cnt_q + 1'b1;
+  // When the LockstepOffset counter value is reached, activate the lockstep
+  // comparison. We do not explicitly check whether rst_shadow_set forms a valid


Nit: Does this need to say "rst_shadow_set_q" here?

Currently, the dual-core lockstep FI mitigation is enabled/disabled using a single bit. For transient bit-flips, this is not problematic, as one bit-flip into this signal and one bit into the Ibex is required to threaten the security of the system. However, a permanent stuck-at-0 fault could disable the lockstep completely by targeting this signal. Then, only a single, additional fault (transient or permanent) is required. This PR enhances the FI resilience of the Ibex lockstep by encoding this single bit into a ibex_mubi_t signal, i.e., a 4-bit multi-bit signal. Signed-off-by: Pascal Nasahl <nasahlpa@lowrisc.org>

andreaskurth

LGTM, thanks for implementing our feedback @nasahlpa 👍

vogelpi

LGTM, thanks @nasahlpa !

nasahlpa force-pushed the ibex_lockstep branch 4 times, most recently from 7143a50 to 2fbf046 Compare January 17, 2024 13:06

nasahlpa mentioned this pull request Jan 17, 2024

[rv_core_ibex] Switch single to multi-bit encoding for Lockstep Enable lowRISC/opentitan#20779

Closed

nasahlpa requested review from msfschaffner, GregAC and vogelpi January 18, 2024 07:45

nasahlpa marked this pull request as ready for review January 18, 2024 07:47

nasahlpa commented Jan 18, 2024

View reviewed changes

andreaskurth reviewed Jan 18, 2024

View reviewed changes

nasahlpa force-pushed the ibex_lockstep branch 2 times, most recently from 5a6b9a3 to 2f040eb Compare January 18, 2024 12:04

nasahlpa mentioned this pull request Jan 18, 2024

Upgrade Verilator dependency #2131

Closed

vogelpi reviewed Jan 18, 2024

View reviewed changes

andreaskurth mentioned this pull request Jan 19, 2024

Update lowrisc_ibex to lowRISC/ibex@123d46b4 lowRISC/opentitan#20889

Closed

nasahlpa force-pushed the ibex_lockstep branch 3 times, most recently from 818b3fa to ba803be Compare January 19, 2024 15:02

nasahlpa commented Jan 19, 2024

View reviewed changes

rtl/ibex_lockstep.sv Show resolved Hide resolved

nasahlpa force-pushed the ibex_lockstep branch from ba803be to 3230961 Compare January 19, 2024 15:55

nasahlpa mentioned this pull request Jan 19, 2024

Update verilator version #2133

Merged

nasahlpa force-pushed the ibex_lockstep branch from 3230961 to 21e6c63 Compare January 19, 2024 17:00

nasahlpa commented Jan 19, 2024

View reviewed changes

rtl/ibex_lockstep.sv Show resolved Hide resolved

nasahlpa force-pushed the ibex_lockstep branch from 21e6c63 to b18d656 Compare January 19, 2024 17:26

nasahlpa force-pushed the ibex_lockstep branch from b18d656 to 05505c3 Compare January 19, 2024 17:28

nasahlpa mentioned this pull request Jan 19, 2024

Upgrade verilator #2134

Open

nasahlpa force-pushed the ibex_lockstep branch from 05505c3 to 99b16bb Compare January 19, 2024 18:59

nasahlpa force-pushed the ibex_lockstep branch from 99b16bb to 779257f Compare January 20, 2024 16:55

rswarbrick approved these changes Jan 20, 2024

View reviewed changes

nasahlpa force-pushed the ibex_lockstep branch from 779257f to 0f69300 Compare January 21, 2024 15:53

andreaskurth approved these changes Jan 22, 2024

View reviewed changes

vogelpi approved these changes Jan 23, 2024

View reviewed changes

vogelpi added this pull request to the merge queue Jan 23, 2024

Merged via the queue into lowRISC:master with commit 8ec0c6f Jan 23, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rtl] Harden lockstep enable against FI #2129

[rtl] Harden lockstep enable against FI #2129

nasahlpa commented Jan 17, 2024 •

edited

Loading

nasahlpa Jan 18, 2024

rswarbrick Jan 18, 2024

nasahlpa Jan 18, 2024

rswarbrick Jan 18, 2024

nasahlpa Jan 18, 2024

andreaskurth left a comment

andreaskurth Jan 18, 2024

nasahlpa Jan 18, 2024

andreaskurth Jan 18, 2024

nasahlpa Jan 18, 2024

andreaskurth Jan 18, 2024

nasahlpa Jan 18, 2024

andreaskurth Jan 18, 2024 •

edited

Loading

vogelpi Jan 18, 2024

vogelpi left a comment

vogelpi Jan 18, 2024

nasahlpa commented Jan 20, 2024

rswarbrick left a comment

rswarbrick Jan 20, 2024

rswarbrick Jan 20, 2024

andreaskurth left a comment

vogelpi left a comment

	.clk0_i(rst_shadow_set_single_bit),
	.clk0_i(rst_shadow_set_q[0]),

		assign rst_shadow_set_d =
		(rst_shadow_cnt_q == LockstepOffsetW'(LockstepOffset - 1)) ? IbexMuBiOn : IbexMuBiOff;

[rtl] Harden lockstep enable against FI #2129

[rtl] Harden lockstep enable against FI #2129

Conversation

nasahlpa commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasahlpa commented Jan 20, 2024

rswarbrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth left a comment

Choose a reason for hiding this comment

vogelpi left a comment

Choose a reason for hiding this comment

nasahlpa commented Jan 17, 2024 •

edited

Loading

andreaskurth Jan 18, 2024 •

edited

Loading