Conversation
vogelpi
left a comment
There was a problem hiding this comment.
Thanks for the PR @andrea-caforio ! I did a first pass and left some comments. Specifically to your questions:
- The digest field of a partition is used to indicate whether a partition is in a zeroized state by counting the number of set bits in its bit representation. This has the side effect that ECC must be disabled for all digest fields since they are now read first upon initialization. If this is undesirable, we need to revert back to a separate zeroization field.
I don't think we should ignore ECC bits on the digest unless it's zeroized. Can we instead do the following procedure?
- Read the digest with ECC disabled to check if the partition is zeroized.
- If it's not, read the entire partition and at the end the digest including ECC (as in the existing implementation)?
- The new OTP_CTRL_DAI_ZER_READ (verification of zeroization) can now read the entire address space of a partition. ...
I left a comment directly in the code regarding how this could be addressed.
- There is a potential race condition that can occur during zeroization of a buffered partition when it is interrupted by a periodical consistency check. These checks are currently only disabled after a reset (when zeroization is detected).
- Transfer the partition into a terminal FSM state upon the first successful OTP_CTRL_DAI_ZER_WRITE command?
The specification discusses a similar issue related to the life cycle partition and life cycle state updates. The challenge here is that at the point where the is zeroization started by the first DAI command and between the last zeroization write (or the reset to trigger re-reading the zeroized digest).
I think it could be solved by adding a mubi register that is written upon detecting a zeroized digest or the first zeroize write (whatever comes first) which is then used to ignore all integrity and consistency check requests. For my own understanding: the integrity checks can proceed as long as the zeroization isn't finished yet, right (because they operate exclusively on the buffer)?
a631755 to
d3e74d4
Compare
|
Thank you @vogelpi for the thorough review. The PR is ready for a second round. :-) |
|
Concerning the race condition. The first successful zeroization request to a partition will periodic consistency check for this partition. |
vogelpi
left a comment
There was a problem hiding this comment.
Thanks @andrea-caforio , I am not done yet and will continue with the review later.
vogelpi
left a comment
There was a problem hiding this comment.
Some more comments but still not finished. Will continue later.
d3e74d4 to
d44c072
Compare
|
@vogelpi Thanks for the 2nd-round of review. With the exception of one point (see comment), I hope everything else |
d44c072 to
2266797
Compare
vogelpi
left a comment
There was a problem hiding this comment.
Thanks @andrea-caforio for the updates. I have some questions remaining. This is looking good.
2266797 to
d8abd98
Compare
vogelpi
left a comment
There was a problem hiding this comment.
Thanks @andrea-caforio for addressing the remaining comments. This LGTM!
As discussed over email, we should add a separate field per partition to mark them as zeroized rather than using the digest (to avoid collisions - especially when defining a non-zero margin). But this can be done as a separate PR in my view.
98c47eb to
d75877d
Compare
|
I'm a bit concerned about the use of I suspect logic [$bits(ScrmblBlockWidth)-1:0] count;
count = 0;
for (int ii = 0 ; ii < ScrmblBlockWidth ; ii++) begin
count = count + otp_rdata_i[ii];
endHopefully this will be synthesised as a tree of adders. First adding 32 pairs of single bits, then adding 16 pairs of those two-bit partial sums, 8 pairs of three-bit part-sums, 4 pairs of four-bit part-sums, 2 pairs of five-bit part-sums, and a final add of a pair of 6-bit part-sums to reach the final single result. Such a large adder tree may unfortunately take up a sizable bit of chip area and path delay. If you are simply checking that all bits are set (as currently seems to be the case) then it would be far better to use a reduction AND operation: Another way to reduce the chip area could be to split the 'countones' adder-tree into a separate module and only instantiate one that is shared by all partitions somehow. You may be able to optimise this further by turning it into a multi-cycle operation and greatly reducing the size of the adder tree. There may also be more efficient ways to synthesise |
|
@elliotb-lowrisc The logarithmic $countones you describe should work, may be required given #579. The largest number of adders are quite small so it may not be that terrible. We could also stop adding at some level, for example given the counts for 16-bit chunks, and decide on that partial information. For example, and based on #579, we could flag "fatal" if more than one has a count of 2, and "okay" if none has more than 1. |
|
@matutem Ah, I had missed that the zeroization test logic had moved from being in each partition to being a single instance in the DAI. The impact on chip area shouldn't be nearly as bad as I previously thought. |
|
Or was I still mistaken? Perhaps it is in both the DAI and each partition. My apologies. I am still concerned about the chip area impact then. |
elliotb-lowrisc
left a comment
There was a problem hiding this comment.
I've not yet fully come to grips with these changes (or OTP in general), but I have found a few things I hope will be useful to point out.
af0ddbd to
5acaca7
Compare
Upon the first successful zeroization request to a partition, all further consistency checks will not execute anymore. Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
* [fuse_ctrl, rtl] Zeroization fuse Use a dedicated zeroization field instead of the digest as the zeroization marker. The field is inserted after the digest when a partition is labelled as zeroizable. Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> * [fuse_ctrl, doc] Zeroization programmer's guide Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> * [fuse_ctrl, mmap] Ratchet seed partitions (#627) * [fuse_ctrl, mmap] Add ratchet seed partitions Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> * [OCP LOCK] Add FC Ratchet Seed Memory Map. The Fuse Controller memory map is updated to include a reference implementation of the OCP L.O.C.K Hard Epoch Key (HEK) ratchet seeds. Each ratchet seed is 256-bits, and is stored in a software partition to make it accessible to MCU or Core firmware. The seed is wrapped by an obfuscation key, making it safe to be read from the MCU. Each ratchet seed is stored in a separate fuse partition to provide the following properties: * Write locking: At fuse programming time by writing a non-zero value to the partition's digest field. The digest is written to fuses. * Read locking: By write-lockable CSR. The lock is held until the next SS reset. * Zeroization: Managed by software. The partitions are placed at the end of the memory map to allow integrators to remove the partition when OCP L.O.C.K functionality is not required. The naming convention uses the partition name as the prefix for each ratchet seed. This is to also make the autogeneration of digest and zeroization fields more ergonomic for firmware - e.g. `CPTRA_SS_LOCK_HEK_PROD_0_DIGEST` corresponds to the digest field for the `CPTRA_SS_LOCK_HEK_PROD_0` partition. Signed-off-by: Miguel Osorio <miguelosorio@google.com> (cherry picked from commit 04d1160) * [fuse_ctrl, rtl] Deassert token valid signal upon zeroization Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> * [fuse_ctrl, rtl] Remove obsolete zeroization logic Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> --------- Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Signed-off-by: Miguel Osorio <miguelosorio@google.com> Co-authored-by: Miguel Osorio <miguelosorio@google.com> * [fuse_ctrl, doc] Zeroization hardware changes documentation Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> --------- Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Signed-off-by: Miguel Osorio <miguelosorio@google.com> Co-authored-by: Miguel Osorio <miguelosorio@google.com>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Only ratchet seed partitions and the vendor secret partition are zeroizable. Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
According to Emre, this is only needed for the partitions that contain the Unique Device Secret (UDS) and field entropy (FE). Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
…zing Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
...egration/test_suites/smoke_test_fc_ocp_lock_zeroization/smoke_test_fc_ocp_lock_zeroization.c
Show resolved
Hide resolved
...egration/test_suites/smoke_test_fc_ocp_lock_zeroization/smoke_test_fc_ocp_lock_zeroization.c
Show resolved
Hide resolved
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org> Co-authored-by: Andreas Kurth <adk@lowrisc.org>
…ck_token` Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
Signed-off-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andreas Kurth <adk@lowrisc.org>
Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
…dated timestamp and hash after successful run
…dated timestamp and hash after successful run
This is a functional (smoke tests not included) draft PR of the zeroization mechanism.
Open questions:
counting the number of set bits in its bit representation. This has the side effect that ECC must
be disabled for all digest fields since they are now read first upon initialization. If this is
undesirable, we need to revert back to a separate zeroization field.
OTP_CTRL_DAI_ZER_READ(verification of zeroization) can now read the entireaddress space of a partition. This is unproblematic for software partitions but for hardware partition it
will return the scrambled data of secret partitions. If this is undesirable, a mechanism needs to
be added to only allow reads of zeroized fuses. For example:
partition when it is interrupted by a periodical consistency check. These checks are
currently only disabled after a reset (when zeroization is detected).
OTP_CTRL_DAI_ZER_WRITEcommand?