[sival, keymgr] State-agnostic chip_sw_key_derivation_test #24755
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrea-caforio
force-pushed
the
sival-key-derivation-test-bugfix
branch
2 times, most recently
from
68a488a
to
7033770
Compare
vogelpi
approved these changes
Oct 10, 2024
There was a problem hiding this comment.
Thanks @andrea-caforio for working on this! The PR looks good, except that I would factor out the removal of unneeded includes into a separate commit. The reason is that even if the test code changes would need reverting in the future, we would want to keep the unneeded includes removed.
But you don't need to do this right now, I think this can be included with potential feedback from the software team. It would be great to also get this approved by e.g. @engdoreis (OoO today) or maybe @jwnrt . I'll ask him.
jwnrt
approved these changes
Oct 10, 2024
Most includes in sw/device/tests/keymgr_key_derivation_test.c are superfluous. This commit removes them and updates the corresponding Bazel build directive. Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
keymgr_key_derivation_test_fpga_cw310_sival_rom_ext test was marked as broken in lowRISC#21706. This commit fixes this and addresses lowRISC#21500. The source of the bug is due to the nature of the ROM_EXT that runs through a CDI attestation flow advancing the key manager state to "OwnerRootKey" in the process. This was not accounted for in the test, which assumed an active "CreatorRootKey" state (this works for sim_dv tests but not with ROM_EXT). The solution to this issue lies in rendering the test state-agnostic, meaning it will execute its verifications independently of the current key manager state. This approach thus mimics the keymgr_sideload_{aes,kmac,otbn} tests which follow a similar strategy. Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>
andrea-caforio
force-pushed
the
sival-key-derivation-test-bugfix
branch
from
7033770
to
aa4031d
Compare
|
Thanks, I'll merge once CI has finished |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
keymgr_key_derivation_test_fpga_cw310_sival_rom_ext testwas marked as broken in #21706. This commit fixes this and addresses #21500.The source of the bug is due to the nature of the ROM_EXT that runs through a CDI attestation flow advancing the key manager state to "OwnerRootKey" in the process. This was not accounted for in the test, which assumed an active "CreatorRootKey" state (this works for sim_dv tests but not with ROM_EXT). The solution to this issue lies in rendering the test state-agnostic, meaning it will execute its verifications independently of the current key manager state. This approach thus mimics the
keymgr_sideload_{aes,kmac,otbn}tests which follow a similar strategy.