Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hw/ip/aes/aes.core
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ filesets:
- lowrisc:ip:lc_ctrl_pkg
- lowrisc:ip:edn_pkg
files:
- rtl/aes_pkg.sv
- rtl/aes_reg_pkg.sv
- rtl/aes_pkg.sv
- rtl/aes_reg_top.sv
- rtl/aes_core.sv
- rtl/aes_ctr.sv
Expand Down
37 changes: 26 additions & 11 deletions hw/ip/aes/data/aes.hjson
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@
# initial key registers
{ multireg: {
name: "KEY_SHARE0",
resval: "0",
desc: '''
Initial Key Registers Share 0.
The actual initial key corresponds to Initial Key Registers Share 0 XORed with Initial Key Registers Share 1.
Expand All @@ -192,6 +193,7 @@
The order in which the registers are updated does not matter.
Can only be updated when the AES unit is idle.
If the AES unit is non-idle, writes to these registers are ignored.
Upon reset, these registers are cleared with pseudo-random data.
'''
count: "NumRegsKey",
cname: "KEY_SHARE0",
Expand All @@ -206,6 +208,7 @@
},
{ multireg: {
name: "KEY_SHARE1",
resval: "0",
desc: '''
Initial Key Registers Share 1.
The actual initial key corresponds to Initial Key Registers Share 0 XORed with Initial Key Registers Share 1.
Expand All @@ -214,6 +217,7 @@
The order in which the registers are updated does not matter.
Can only be updated when the AES unit is idle.
If the AES unit is non-idle, writes to these registers are ignored.
Upon reset, these registers are cleared with pseudo-random data.
'''
count: "NumRegsKey",
cname: "KEY_SHARE1",
Expand All @@ -230,6 +234,7 @@
# initialization vector registers
{ multireg: {
name: "IV",
resval: "0",
desc: '''
Initialization Vector Registers.
The initialization vector (IV) or initial counter value must be written to these registers when starting a new message in CBC or CTR mode (see Control Register), respectively.
Expand All @@ -240,6 +245,7 @@
Whenever starting a new message, the corresponding IV value must be provided by the processor.
Once started, the AES unit automatically updates the contents of these registers.
In ECB mode, the IV registers are not used and do not need to be configured.
Upon reset, these registers are cleared with pseudo-random data.
'''
count: "NumRegsIv",
cname: "IV",
Expand All @@ -256,13 +262,15 @@
# input data registers
{ multireg: {
name: "DATA_IN",
resval: "0",
desc: '''
Input Data Registers.
If MANUAL_OPERATION=0 (see Control Register), the AES unit automatically starts encryption/decryption after these register have been written.
If MANUAL_OPERATION=0 (see Control Register), the AES unit automatically starts encryption/decryption after all Input Data registers have been written.
Each register has to be written at least once.
The order in which the registers are written does not matter.
Loaded into the internal State register upon starting encryption/decryption of the next block.
After that, the processor can update the Input Data Register.
After that, the processor can update the Input Data registers (See INPUT_READY field of Status Register).
Upon reset, these registers are cleared with pseudo-random data.
'''
count: "NumRegsData",
cname: "DATA_IN",
Expand All @@ -278,12 +286,14 @@
# output data registers
{ multireg: {
name: "DATA_OUT",
resval: "0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vogelpi I think we need to be even more clear on this.
the reset value is only true right after reset - when the internal initialization is done the value of data_out will no longer be the reset value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I've added a comment to all these registers saying that upon reset they are cleared with pseudo-random data. Previously we just had a corresponding comment to motivate the CSR test exclusion, but this was not visible in the online documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer "they are initialized with pseudo-random data", since "cleared" strongly suggests zero filling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion either way, but we typically used the term "secure clear" to mean "overwriting with random data" in other components of OpenTitan as well (e.g. in OTBN). I see the reason not to call this operation "clear", maybe we can come up with consistent naming that we can then also apply to other parts of the system?

What do others think? @cdgori @tjaychen @moidx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why we initialize these registers with pseudo-random data instead of just setting them to zero. If we zero them it may be possible to be idle right after reset, which could be simpler overall, and zero leaks no information. Do you get the pseudo-random data from some other unit? If AES reset depends on other units we could be inviting trouble.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just learning this, so please bear with me... Since the potential problem is leaking the hamming distance to zero I imagine "secure wiping" with the local prng may be enough with no need for EDN. If upon non-POR the local PRNG is already sufficiently random because it was seeded by the entropy source at least once then do we still need to depend on EDN for reset?

Copy link

@cdgori cdgori Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matutem no problem, you've got lots of fun topics to learn about ;) - welcome to the party!

The EDN dependency is only for reseeding the local PRNG, because if not reseeded with some (hand-waving here, because it's hard to say) periodicity, then the PRNG could be predicted and we would leak the Hamming distance when clearing (since the adversary would know the wipe value, in theory). But our general policy for this sort of thing is that if EDN is stalled for some reason and cannot supply a reseed (even if the client block/PRNG thinks it should be reseeding), it's better to just do the "secure wipe" immediately rather than wait around for EDN to reseed. So I don't think there is a "hard" dependency on EDN for non-POR cases. If you are seeing one we should discuss more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cdgori for explaining the motivation for this. I have nothing to add here.

Regarding terminology: I very much prefer the same terminology being used between documentation, RTL and DV. I've seen other examples and this can be very confusing. We have now been using "clearing with pseudo-random data" in AES for more than a year without issues. Today we have 296 occurrences of "clear" in the RTL, another 93 in DV. It's in signal names, FSM states, enum values, comments etc. etc. If we are going to change this, the whole code needs to be reviewed again, also from a security perspective. To be honest, I think our and especially @cdgori 's time is better invested in more critical stuff.

I am open to adapt this if we want to use consistent naming across the project. But before I will implement any such changes, we have to first find wider agreement and then write up a recommendation somewhere (e.g. in the comportability spec). I am not willing to do this without formalized recommendation as last time we had a similar discussion (alert naming scheme) me and Chris ended up doing the work twice.

Anway, such naming scheme discussions should not block this PR. If we want to go for it, we should open a seperate issue and start it from there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I am seeing: I am trying to disable the AES clock from clkmgr after POR, but the AES is not idle (as expected). But the AES control fsm transitions to PRNG_RESEED, and it is sending out an entropy request. My test is set to time out after 10ms, which I thought would be more than enough to secure wipe the registers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About how long will it take for the AES to go idle?

desc: '''
Output Data Register.
Holds the output data produced by the AES unit during the last encryption/decryption operation.
If MANUAL_OPERATION=0 (see Control Register), the AES unit is stalled when the previous output data has not yet been read and is about to be overwritten.
Each register has to be read at least once.
The order in which the registers are read does not matter.
Upon reset, these registers are cleared with pseudo-random data.
'''
count: "NumRegsData",
cname: "DATA_OUT",
Expand Down Expand Up @@ -324,6 +334,7 @@
fields: [
{ bits: "0",
name: "OPERATION",
resval: "0",
desc: '''
Select encryption(0) or decryption(1) operation of AES unit.
'''
Expand Down Expand Up @@ -408,6 +419,7 @@
}
{ bits: "10",
name: "MANUAL_OPERATION",
resval: "0"
desc: '''
Controls whether the AES unit is operated in normal/automatic mode (0) or fully manual mode (1).
In automatic mode (0), the AES unit automatically i) starts to encrypt/decrypt when it receives new input data, and ii) stalls during the last encryption/decryption cycle if the previous output data has not yet been read.
Expand Down Expand Up @@ -494,13 +506,13 @@
# Tag info (CSR test exclusions):
# Updated by the HW.
# Updates based on writes to other regs.
# -> Exclude all fields (except ALERT_FATAL) from init and write-read checks.
# Upon reset, internal operations are triggered that temporarily change the IDLE field.
# -> Exclude IDLE field from init and write-read checks (also in reset test).
# -> Exclude all fields (except ALERT_FATAL_FAULT) from init and write-read checks.
# Upon reset, internal operations are triggered at the end of which IDLE and INPUT_READY will be 1.
# -> Exclude IDLE and INPUT_READY field from init and write-read checks (also in reset test).
fields: [
{ bits: "0",
name: "IDLE",
resval: "1",
resval: "0",
desc: '''
The AES unit is idle (1) or busy (0).
This flag is `0` if one of the following operations is currently running: i) encryption/decryption, ii) register clearing or iii) PRNG reseeding.
Expand All @@ -510,6 +522,7 @@
}
{ bits: "1",
name: "STALL",
resval: "0"
desc: '''
The AES unit is not stalled (0) or stalled (1) because there is previous
output data that must be read by the processor before the AES unit can
Expand All @@ -520,6 +533,7 @@
}
{ bits: "2",
name: "OUTPUT_LOST",
resval: "0"
hwaccess: "hrw",
desc: '''
All previous output data has been fully read by the processor (0) or at least one previous output data block has been lost (1).
Expand All @@ -532,20 +546,21 @@
}
{ bits: "3",
name: "OUTPUT_VALID",
resval: "0"
desc: '''
The AES unit has no valid output (0) or has valid output data (1).
'''
tags: ["excl:CsrNonInitTests:CsrExclCheck"]
}
{ bits: "4",
name: "INPUT_READY",
resval: "1",
resval: "0",
desc: '''
The AES unit is ready (1) to receive new data input via the DATA_IN registers or
the present values in the DATA_IN registers have not yet been loaded into the
module (0).
The AES unit is ready (1) or not ready (0) to receive new data input via the DATA_IN registers.
If the present values in the DATA_IN registers have not yet been loaded into the
module this flag is `0` (not ready).
'''
tags: ["excl:CsrNonInitTests:CsrExclCheck"]
tags: ["excl:CsrAllTests:CsrExclCheck"]
}
{ bits: "5",
name: "ALERT_RECOV_CTRL_UPDATE_ERR",
Expand Down
9 changes: 9 additions & 0 deletions hw/ip/aes/doc/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,14 @@ Future versions of this AES unit thus might employ different means at architectu
This section discusses how software can interface with the AES unit.


## Clear upon Reset

Upon reset, the AES unit will first reseed the internal PRNG for register clearing via EDN, and then clear all key, IV and data registers with pseudo-random data.
Only after this sequence has finished, the unit becomes idle (indicated in {{< regref "STATUS.IDLE" >}}).
The AES unit is then ready for software initialization.
Note that at this point, the key, IV and data registers' values can no longer be expected to match the reset values.


## Initialization

Before initialization, software must ensure that the AES unit is idle by checking {{< regref "STATUS.IDLE" >}}.
Expand Down Expand Up @@ -364,6 +372,7 @@ It ensures that the AES unit:
1. Does not overwrite previous output data that has not yet been read by the processor.

Then, software must:
1. Ensure that the INPUT_READY bit in {{< regref "STATUS" >}} is `1`.
1. Write Input Data Block `0` to the Input Data registers {{< regref "DATA_IN_0" >}} - {{< regref "DATA_IN_3" >}}.
Each register must be written at least once.
The order in which these registers are written does not matter.
Expand Down
10 changes: 5 additions & 5 deletions hw/ip/aes/rtl/aes_pkg.sv
Original file line number Diff line number Diff line change
Expand Up @@ -316,11 +316,11 @@ typedef struct packed {
} ctrl_reg_t;

parameter ctrl_reg_t CTRL_RESET = '{
force_zero_masks: '0,
manual_operation: '0,
key_len: AES_128,
mode: AES_NONE,
operation: AES_ENC
force_zero_masks: aes_reg_pkg::AES_CTRL_SHADOWED_FORCE_ZERO_MASKS_RESVAL,
manual_operation: aes_reg_pkg::AES_CTRL_SHADOWED_MANUAL_OPERATION_RESVAL,
key_len: key_len_e'(aes_reg_pkg::AES_CTRL_SHADOWED_KEY_LEN_RESVAL),
mode: aes_mode_e'(aes_reg_pkg::AES_CTRL_SHADOWED_MODE_RESVAL),
operation: aes_op_e'(aes_reg_pkg::AES_CTRL_SHADOWED_OPERATION_RESVAL)
};

// Multiplication by {02} (i.e. x) on GF(2^8)
Expand Down
26 changes: 26 additions & 0 deletions hw/ip/aes/rtl/aes_reg_pkg.sv
Original file line number Diff line number Diff line change
Expand Up @@ -257,32 +257,58 @@ package aes_reg_pkg;
parameter logic [0:0] AES_ALERT_TEST_RECOV_CTRL_UPDATE_ERR_RESVAL = 1'h 0;
parameter logic [0:0] AES_ALERT_TEST_FATAL_FAULT_RESVAL = 1'h 0;
parameter logic [31:0] AES_KEY_SHARE0_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_0_KEY_SHARE0_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_1_KEY_SHARE0_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_2_KEY_SHARE0_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_3_KEY_SHARE0_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_4_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_4_KEY_SHARE0_4_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_5_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_5_KEY_SHARE0_5_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_6_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_6_KEY_SHARE0_6_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_7_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE0_7_KEY_SHARE0_7_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_0_KEY_SHARE1_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_1_KEY_SHARE1_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_2_KEY_SHARE1_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_3_KEY_SHARE1_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_4_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_4_KEY_SHARE1_4_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_5_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_5_KEY_SHARE1_5_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_6_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_6_KEY_SHARE1_6_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_7_RESVAL = 32'h 0;
parameter logic [31:0] AES_KEY_SHARE1_7_KEY_SHARE1_7_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_0_IV_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_1_IV_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_2_IV_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_IV_3_IV_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_0_DATA_OUT_0_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_1_DATA_OUT_1_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_2_DATA_OUT_2_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_3_RESVAL = 32'h 0;
parameter logic [31:0] AES_DATA_OUT_3_DATA_OUT_3_RESVAL = 32'h 0;
parameter logic [11:0] AES_CTRL_SHADOWED_RESVAL = 12'h c0;
parameter logic [0:0] AES_CTRL_SHADOWED_OPERATION_RESVAL = 1'h 0;
parameter logic [5:0] AES_CTRL_SHADOWED_MODE_RESVAL = 6'h 20;
parameter logic [2:0] AES_CTRL_SHADOWED_KEY_LEN_RESVAL = 3'h 1;
parameter logic [0:0] AES_CTRL_SHADOWED_MANUAL_OPERATION_RESVAL = 1'h 0;
parameter logic [0:0] AES_CTRL_SHADOWED_FORCE_ZERO_MASKS_RESVAL = 1'h 0;

// Register index
Expand Down
4 changes: 2 additions & 2 deletions hw/ip/aes/rtl/aes_reg_top.sv
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,7 @@ module aes_reg_top (
prim_subreg #(
.DW (1),
.SWACCESS("RO"),
.RESVAL (1'h1)
.RESVAL (1'h0)
) u_status_idle (
.clk_i (clk_i ),
.rst_ni (rst_ni ),
Expand Down Expand Up @@ -1012,7 +1012,7 @@ module aes_reg_top (
prim_subreg #(
.DW (1),
.SWACCESS("RO"),
.RESVAL (1'h1)
.RESVAL (1'h0)
) u_status_input_ready (
.clk_i (clk_i ),
.rst_ni (rst_ni ),
Expand Down