[aes] Rework handling and signaling of invalid mux selector signals
This commit reworks the handling of invalid mux selector signals and how
they are signaled from the cipher core to the main controller.
In case of an invalid mux selector signal inside the cipher core, we no
longer immediately signal the error to the main controller FSM. Instead, we
just make sure no data is released from the cipher core. The alert is then
signaled in the next clock cycle once the terminal error state has been
entered. This avoids timing loops occurring during FPGA synthesis.
Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
diff --git a/hw/ip/aes/lint/aes.waiver b/hw/ip/aes/lint/aes.waiver
index 840f0d9..ec00b61 100644
--- a/hw/ip/aes/lint/aes.waiver
+++ b/hw/ip/aes/lint/aes.waiver
@@ -17,3 +17,15 @@
waive -rules {RESET_USE} -location {aes_key_expand.sv} -regexp {'rst_ni' is connected to 'aes_sbox' port} \
-comment "when using fully combinational S-Box implementations, no reset is used inside aes_sbox"
+
+waive -rules {CLOCK_USE} -location {aes_core.sv} -regexp {clk_i' is connected to 'aes_sel_buf_chk' port} \
+ -comment "clock and reset are just used for assertions inside aes_sel_buf_chk"
+
+waive -rules {CLOCK_USE} -location {aes_cipher_core.sv} -regexp {clk_i' is connected to 'aes_sel_buf_chk' port} \
+ -comment "clock and reset are just used for assertions inside aes_sel_buf_chk"
+
+waive -rules {RESET_USE} -location {aes_core.sv} -regexp {'rst_ni' is connected to 'aes_sel_buf_chk' port} \
+ -comment "clock and reset are just used for assertions inside aes_sel_buf_chk"
+
+waive -rules {RESET_USE} -location {aes_cipher_core.sv} -regexp {'rst_ni' is connected to 'aes_sel_buf_chk' port} \
+ -comment "clock and reset are just used for assertions inside aes_sel_buf_chk"
diff --git a/hw/ip/aes/rtl/aes_cipher_control.sv b/hw/ip/aes/rtl/aes_cipher_control.sv
index 6237284..5c03794 100644
--- a/hw/ip/aes/rtl/aes_cipher_control.sv
+++ b/hw/ip/aes/rtl/aes_cipher_control.sv
@@ -177,18 +177,18 @@
dec_key_gen_d = dec_key_gen_i;
// Load input data to state
- state_sel_o = dec_key_gen_d ? STATE_CLEAR : STATE_INIT;
+ state_sel_o = dec_key_gen_i ? STATE_CLEAR : STATE_INIT;
state_we_o = 1'b1;
// Make the masking PRNG advance. The current pseudo-random data is used to mask the
// input data.
- prng_update_o = dec_key_gen_d ? 1'b0 : Masking;
+ prng_update_o = dec_key_gen_i ? 1'b0 : Masking;
// Init key expand
key_expand_clear_o = 1'b1;
// Load full key
- key_full_sel_o = dec_key_gen_d ? KEY_FULL_ENC_INIT :
+ key_full_sel_o = dec_key_gen_i ? KEY_FULL_ENC_INIT :
(op_i == CIPH_FWD) ? KEY_FULL_ENC_INIT :
KEY_FULL_DEC_INIT;
key_full_we_o = 1'b1;
@@ -311,19 +311,24 @@
// Skip mix_columns
add_rk_sel_o = ADD_RK_FINAL;
+ // Once we're done, we won't need the state anymore. We actually clear it when progressing
+ // to the next state.
+ state_sel_o = STATE_CLEAR;
+
// Advance in sync with SubBytes. Based on the S-Box implementation, it can take multiple
// cycles to finish. Only indicate that we are done if:
- // - we have valid output (SubBytes finished), and
- // - the masking PRNG has been reseeded (if masking is used).
+ // - we have valid output (SubBytes finished),
+ // - the masking PRNG has been reseeded (if masking is used), and
+ // - all mux selector signals are valid (don't release data in that case of errors).
// Perform both handshakes simultaneously.
sub_bytes_en_o = ~dec_key_gen_q;
- out_valid_o = (dec_key_gen_q | sub_bytes_out_req_i) & (Masking == prng_reseed_done_q);
+ out_valid_o = (dec_key_gen_q | sub_bytes_out_req_i) & (Masking == prng_reseed_done_q) &
+ ~mux_sel_err_i;
if (out_valid_o && out_ready_i) begin
sub_bytes_out_ack_o = ~dec_key_gen_q;
- // We don't need the state anymore, clear it.
+ // Clear the state.
state_we_o = 1'b1;
- state_sel_o = STATE_CLEAR;
crypt_d = 1'b0;
// Make the masking PRNG advance once only. Updating it while being stalled would
// cause the SBoxes to be re-evaluated, thereby creating additional SCA leakage.
@@ -372,7 +377,6 @@
// We should never get here. If we do (e.g. via a malicious glitch), error out immediately.
default: begin
- alert_o = 1'b1;
aes_cipher_ctrl_ns = ERROR;
end
endcase
@@ -380,7 +384,6 @@
// Unconditionally jump into the terminal error state in case a mux selector signal becomes
// invalid.
if (mux_sel_err_i) begin
- alert_o = 1'b1;
aes_cipher_ctrl_ns = ERROR;
end
end
diff --git a/hw/ip/aes/rtl/aes_cipher_core.sv b/hw/ip/aes/rtl/aes_cipher_core.sv
index 329909d..71b0798 100644
--- a/hw/ip/aes/rtl/aes_cipher_core.sv
+++ b/hw/ip/aes/rtl/aes_cipher_core.sv
@@ -513,8 +513,12 @@
// 1. The synthesis tool doesn't optimize away the sparse encoding.
// 2. The selector signal is always valid. More precisely, an alert or SVA is triggered if a
// selector signal takes on an invalid value.
- // 3. The error/alert signal remains asserted until reset even if the selector signal becomes
- // valid again.
+ // 3. The alert signal remains asserted until reset even if the selector signal becomes valid
+ // again. This is achieved by driving the control FSM into the terminal error state whenever
+ // any mux selector signal becomes invalid.
+ //
+ // If any mux selector signal becomes invalid, the cipher core further immediately de-asserts
+ // the out_valid_o signal to prevent any data from being released.
aes_sel_buf_chk #(
.Num ( StateSelNum ),
diff --git a/hw/ip/aes/rtl/aes_control.sv b/hw/ip/aes/rtl/aes_control.sv
index d9b9016..c0c4d12 100644
--- a/hw/ip/aes/rtl/aes_control.sv
+++ b/hw/ip/aes/rtl/aes_control.sv
@@ -478,7 +478,8 @@
// Proceed upon successful handshake.
if (cipher_out_done) begin
- data_out_we_o = 1'b1;
+ // Don't release data from cipher core in case of invalid mux selector signals.
+ data_out_we_o = ~mux_sel_err_i;
aes_ctrl_ns = IDLE;
end
end
@@ -517,8 +518,9 @@
// To clear the output data registers, we re-use the muxing resources of the cipher core.
// data_out_clear_i is acknowledged by the cipher core with cipher_data_out_clear_i.
if (cipher_data_out_clear_i) begin
- // Clear output data and the trigger bit.
- data_out_we_o = 1'b1;
+ // Clear output data and the trigger bit. Don't release data from cipher core in case
+ // of invalid mux selector signals.
+ data_out_we_o = ~mux_sel_err_i;
data_out_clear_we_o = 1'b1;
end
@@ -533,7 +535,6 @@
// We should never get here. If we do (e.g. via a malicious glitch), error out immediately.
default: begin
- alert_o = 1'b1;
aes_ctrl_ns = ERROR;
end
endcase
@@ -541,7 +542,6 @@
// Unconditionally jump into the terminal error state in case a mux selector signal becomes
// invalid.
if (mux_sel_err_i) begin
- alert_o = 1'b1;
aes_ctrl_ns = ERROR;
end
end
@@ -560,6 +560,10 @@
.q_o ( aes_ctrl_cs_raw )
);
+ /////////////////////
+ // Status Tracking //
+ /////////////////////
+
// We only use clean initial keys. Either software/counter has updated
// - all initial key registers, or
// - none of the initial key registers but the registers were updated in the past.
@@ -672,6 +676,10 @@
assign data_out_clear_o = 1'b0;
assign prng_reseed_o = 1'b0;
+ ////////////////
+ // Assertions //
+ ////////////////
+
// Selectors must be known/valid
`ASSERT(AesModeValid, !ctrl_err_storage_i |-> mode_i inside {
AES_ECB,
diff --git a/hw/ip/aes/rtl/aes_core.sv b/hw/ip/aes/rtl/aes_core.sv
index 9305d27..963246c 100644
--- a/hw/ip/aes/rtl/aes_core.sv
+++ b/hw/ip/aes/rtl/aes_core.sv
@@ -597,8 +597,13 @@
// 1. The synthesis tool doesn't optimize away the sparse encoding.
// 2. The selector signal is always valid. More precisely, an alert or SVA is triggered if a
// selector signal takes on an invalid value.
- // 3. The error/alert signal remains asserted until reset even if the selector signal becomes
- // valid again.
+ // 3. The alert signal remains asserted until reset even if the selector signal becomes valid
+ // again. This is achieved by driving the control FSM into the terminal error state whenever
+ // any mux selector signal becomes invalid.
+ //
+ // If any mux selector signal becomes invalid, the control FSM further prevents any data from
+ // being released from the cipher core by de-asserting the write enable of the output data
+ // registers.
aes_sel_buf_chk #(
.Num ( DIPSelNum ),
diff --git a/hw/ip/aes/rtl/aes_ctr.sv b/hw/ip/aes/rtl/aes_ctr.sv
index ee7e839..a116a49 100644
--- a/hw/ip/aes/rtl/aes_ctr.sv
+++ b/hw/ip/aes/rtl/aes_ctr.sv
@@ -139,7 +139,6 @@
// We should never get here. If we do (e.g. via a malicious
// glitch), error out immediately.
default: begin
- alert_o = 1'b1;
aes_ctr_ns = ERROR;
end
endcase
diff --git a/hw/ip/aes/rtl/aes_sel_buf_chk.sv b/hw/ip/aes/rtl/aes_sel_buf_chk.sv
index c1e6ff8..746dc16 100644
--- a/hw/ip/aes/rtl/aes_sel_buf_chk.sv
+++ b/hw/ip/aes/rtl/aes_sel_buf_chk.sv
@@ -5,9 +5,9 @@
// AES mux selector buffer and checker
//
// When using sparse encodings for mux selector signals, this module can be used to:
-// 1. Prevent aggressive synthesis optimizations on the selector signal, to
-// 2. check that the selector signal is valid, i.e., doesn't take on invalid values, and to
-// 3. keep outputting an error until reset even if the selector signal becomes valid again.
+// 1. Prevent aggressive synthesis optimizations on the selector signal, and
+// 2. to check that the selector signal is valid, i.e., doesn't take on invalid values.
+// Whenever the selector signal takes on an invalid value, an error is signaled.
`include "prim_assert.sv"
@@ -24,8 +24,11 @@
import aes_pkg::*;
- // Signals
- logic err, err_d, err_q;
+ // Tie off unused inputs.
+ logic unused_clk;
+ logic unused_rst;
+ assign unused_clk = clk_i;
+ assign unused_rst = rst_ni;
////////////
// Buffer //
@@ -51,13 +54,13 @@
always_comb begin : mux2_sel_chk
unique case (sel_chk)
MUX2_SEL_0,
- MUX2_SEL_1: err = 1'b0;
- default: err = 1'b1;
+ MUX2_SEL_1: err_o = 1'b0;
+ default: err_o = 1'b1;
endcase
end
// Assertion
- `ASSERT(AesMux2SelValid, !err |-> sel_chk inside {
+ `ASSERT(AesMux2SelValid, !err_o |-> sel_chk inside {
MUX2_SEL_0,
MUX2_SEL_1
})
@@ -72,13 +75,13 @@
unique case (sel_chk)
MUX3_SEL_0,
MUX3_SEL_1,
- MUX3_SEL_2: err = 1'b0;
- default: err = 1'b1;
+ MUX3_SEL_2: err_o = 1'b0;
+ default: err_o = 1'b1;
endcase
end
// Assertion
- `ASSERT(AesMux3SelValid, !err |-> sel_chk inside {
+ `ASSERT(AesMux3SelValid, !err_o |-> sel_chk inside {
MUX3_SEL_0,
MUX3_SEL_1,
MUX3_SEL_2
@@ -95,13 +98,13 @@
MUX4_SEL_0,
MUX4_SEL_1,
MUX4_SEL_2,
- MUX4_SEL_3: err = 1'b0;
- default: err = 1'b1;
+ MUX4_SEL_3: err_o = 1'b0;
+ default: err_o = 1'b1;
endcase
end
// Assertion
- `ASSERT(AesMux4SelValid, !err |-> sel_chk inside {
+ `ASSERT(AesMux4SelValid, !err_o |-> sel_chk inside {
MUX4_SEL_0,
MUX4_SEL_1,
MUX4_SEL_2,
@@ -121,13 +124,13 @@
MUX6_SEL_2,
MUX6_SEL_3,
MUX6_SEL_4,
- MUX6_SEL_5: err = 1'b0;
- default: err = 1'b1;
+ MUX6_SEL_5: err_o = 1'b0;
+ default: err_o = 1'b1;
endcase
end
// Assertion
- `ASSERT(AesMux6SelValid, !err |-> sel_chk inside {
+ `ASSERT(AesMux6SelValid, !err_o |-> sel_chk inside {
MUX6_SEL_0,
MUX6_SEL_1,
MUX6_SEL_2,
@@ -138,20 +141,9 @@
end else begin : gen_width_unsupported
// Selected width not supported, signal error.
- assign err = 1'b1;
+ assign err_o = 1'b1;
end
- // Make sure the error remains asserted until reset.
- assign err_d = err | err_q;
- always_ff @(posedge clk_i or negedge rst_ni) begin : reg_err
- if (!rst_ni) begin
- err_q <= '0;
- end else if (err_d) begin
- err_q <= err_d;
- end
- end
- assign err_o = err_q;
-
////////////////
// Assertions //
////////////////
