Conversation
|
Hmm, I've just looked more carefully at the uds test I'm mentioning in the PR message. It turns out that the original version of that test made a bit more sense: it tried to zeroize the secret partitions as the MCU (which would fail) and then handed control over to a different binary, running on the caliptra core (which should be able to zeroize things). That changed dramatically with a commit called [fuse_ctrl] Simplify MCU UDS/FE test (still reachable in the PR with de8e5f8). @moidx: I think that commit was yours. Have I missed a clever thing that the test is still doing? |
martin-velay
left a comment
martin-velay
left a comment
There was a problem hiding this comment.
Thanks for the fixes, changes ans cleanups. I have 3-4 comments though.
|
The force-push just tidies up the comments (thanks, @martin-velay, for the review!). I'll re-trigger the promote pipeline now. |
Hi @rswarbrick, I split the test into two to make execution faster. There is one that checks UDS and FE zeroization from MCU, which is expected to fail ( |
|
Ah brilliant, thanks for explaining. I'll rebase this and re-trigger CI. |
The correct address changed a month ago (with commit c40923d). Maybe hard-coding the literal wasn't such a good idea :-)
This wasn't quite true: the MCU isn't able to do a DAI write of the zeroize marker of a secret partition. Fix things up and add lots of comments to explain what's going on (because it took me a while to understand).
The existing assertion wasn't quite correct: if a zeroization returns an error, it might not have done any zeroizing!
This was prompted by issue #767 (which is completely correct). The fix moves things into the types to make it impossible to make the same mistake again. I've also slightly tweaked check_digest when expected_zeroized is false: if we don't expect to zeroize a fuse, it's still reasonable for *half* of the fuse to get zeroized.
The basic idea doesn't really change behaviour at all, but avoids all of the calls to dai_zer needing to follow up with a read / check of the rdata. As a bonus, I've also made the tests slightly more "fragile" (so that they will exit immediately on an error if they are doing a loop) When getting this working properly, I had to do some work to figure out whether we should expect partition access to work properly. For example, smoke_test_fc_mcu_uds_fe_zeroization has a zeroization_check_unfeasible function which is supposed to try to zeroize a partition and notice that it doesn't work. Here, I have strengthened the checks quite a bit, predicting the DAI status we should expect (so we check we are getting the right error instead of e.g. the command not reporting a problem). Similarly, I had to do a reasonable amount of work to make the code clearer in caliptra_ss_fuse_ctrl_zeroization_reset (after introducing and fixing an exciting array of bugs).
No functional change, but the code gets slightly shorter and the log messages are a bit clearer.
This now prints out a more informative message that shows exactly what we were comparing that didn't match.
No big change, but the "step" indices are now consistent (after I'd messed them up with previous changes).
The prepare_test function writes magic values to the fuses and then checks that these magic values have indeed arrived. That's not really all that necessary though (and takes ages!). This commit switches things so that partitinos after the first written don't bother checking that the values have been written. This won't cause a verification gap, but it might make debugging a failure marginally harder. (I strongly doubt it though)
andreaskurth
force-pushed
the
lowrisc_3
branch
from
3c8d2d2 to
ba27aa6
Compare
|
Rebased on |
…p and hash after successful run
|
Merged from main and restarted promote on this PR since #803 was merged after this one was ready. |
…p and hash after successful run
6 participants
The first few commits contain some fixes to the SVA that was recently put in to check that zeroisation works. What we'd missed is that it might not work (indeed should not work!) if it returns an exit status.
The other large change is [fuse_ctrl,dv] Move check for zeroization result into library code. When I started looking at it a few days ago, I thought it would be easy! I think I've finally got everything working again. When doing that, I made a couple of other improvements.
The minor changes are to
caliptra_ss_fuse_ctrl_zeroization_reset.c: this took ages to run, for no big benefit so I've made it a bit more efficient. The more significant changes are tosmoke_test_fc_mcu_uds_fe_zeroization.c. I'm not convinced it was previously checking responses properly and, when I started checking them, it failed! Honestly, I'm a bit confused about the test, since it's doing a lot of stuff that shouldn't work (and checking it doesn't work) on a secret partition. I intend to spend a bit more time looking at it, but this is definitely an improvement on what was there before.