-
Notifications
You must be signed in to change notification settings - Fork 12
ot_keymgr
: Implement key generation, wiping & better error handling
#188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ot_keymgr
: Implement key generation, wiping & better error handling
#188
Conversation
Progresses lowRISC/opentitan#27894, partially progresses lowRISC/opentitan#27931. |
bf216a4
to
3553394
Compare
bf0707e
to
ae2e9e0
Compare
89a8e08
to
f74a731
Compare
(oops, accidentally didn't mark this as a draft until #185 is merged) |
Correct the behaviour of the keymgr to restore the previous output KMAC sideloading key after finishing any keymgr operation that uses KMAC (for offloading KDF). The sideloading HW output is not yet implemented, but this functionality will be used in future commits which add this. As per the Keymgr docs: "... The KMAC key thus has two possible outputs, one is the sideload key, and the other is internal state key. When a valid operation is called, the internal state key is sent over the KMAC key. During all other times, the sideloaded value is presented..." Implement this in QEMU by just saving the current sideload key to some internal "backup" state on every sideload key push, and after completing an operation we restore this saved key (instead of clearing the sink). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Implements the "Generate HW Output Key" and "Generate SW Output Key" operations for the `keymgr`. As part of this, the Read-Clear (RC) `SW_SHARE{0,1}_OUTPUT{0,7}` registers are also implemented. This is fairly similar to the equivalent `keymgr_dpe` implementation, with the main differences being the use of CDI selections instead of slots, and the addition of appropriate errors on failing data validity checks. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Implementing identity generation is a small addition on top of the previously implemented Advance and Generate SW/HW output key operations. The identity KDF input is just one of the constant seeds imported from the configuration file as a property (depending on the current key stage i.e. `keymgr`` FSM state). The key state is then sent to KMAC as normal, and the output is as with a SW output command. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Improve handling of invalid KMAC inputs using the all-0/1 integrity check functionality, and correct `KMAC_INPUT_ERROR` to only be visible after the current operation has completed by using `valid_inputs`. Refactors `Advance` data validity checks to use this new state so that each check can update its own unique DEBUG register field on failure. Note: this commit still does not implement the KMAC key integrity check, because keys are still wiped/init with 0s instead of random entropy. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Implements basic functionality for clearing the sideloaded keys, which for now just zeroes out the keys, as wiping with entropy is not yet implemented. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Resolves many TODOs in the `keymgr`, focusing on writing internal key states, SW visible output key states and HW sideloaded keys with random data at points in operation when their values are not in use. Importantly, the same entropy is used when wiping different keys, as in the RTL. This also means that when KMAC masking is supported, XORing key shares will still function as intended for the KMAC key. To more closely align with the HW, an option is provided to seed PRNG from the LFSR seed, with a property added to expose this option. Note however that the entropy reseed refresh count is still not implemented; emulating this would introduce a lot of additional complexity for little benefit, so this is left incomplete for now. Now that key wiping is added, we introduce the key validity check and report both errors and debug status related to invalid keys. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Though we likely wouldn't expect to see either of these faults in QEMU when everything is working, modeling these faults helps both more closely align with the actual HW, and should help debugging the QEMU model if errors do occur (e.g a KMAC issue could manifest as a `KMAC_OUT` fault in the `keymgr`, aiding in debugging). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Also document the final known limitations and TODOs for the `keymgr` in the prologue. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Now that the `keymgr` is completely implemented (including sideloading and the generate operations), every keymgr/sideloading test that we can run in the `rom_with_fake_keys` execution environment is now passing. Add these tests to the known test passing list. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
f74a731
to
5725684
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed for style and didn't see any obvious problems. Given that all the keymgr
tests are now passing I trust that the logic is correct as well.
Thanks
} OtKeyMgrKeySink; | ||
|
||
static_assert(KEYMGR_KEY_SINK_COUNT < 32, | ||
"Sideload clear bitmasking logic needs < 32 key sinks"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether it could make sense to define something like QEMU bitops.h
:
#define BITS_PER_BYTE CHAR_BIT
#define BITS_PER_LONG (sizeof (unsigned long) * BITS_PER_BYTE)
for 32-bit values since we use them a lot in hw/opentitan
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - that seems like something that could end up being useful (and avoid mistakes). There are quite a few useful helpers in bitops.h
. In this specific case > 32 key sinks would require a rewrite due to register changes, so I'm really just being extra safe and sanity checking that we don't have exactly 32 keymgr key sinks in our design somehow! (I hope not)
This PR is the fourth of a series of 4 PRs to implement OpenTitan's
keymgr
IP, used in Earlgrey, split to ease review. It finishes implementing the main logic for the keymgr, allowing the generation of both SW output and HW output (sideloaded) keys, as well as identity generation outputs. It implements sideloaded key clearing and fixed handling of the KMAC sideloaded key between operations, and models wiping of the keys with entropy (partially, there is still no entropy reseeding). It also adds some more debug & fault status checks. The docs are then updated with the new keymgr status, noting anything that is still unimplemented.As seen in the last commit, thanks to the additions from previous PRs (otp_ctrl, flash_ctrl, aes, kmac, otbn) this is now enough functionality to get all
sim_qemu_rom_with_fake_keys
OpenTitan tests related tokeymgr
orsideloading
passing.See the commit messages for more details about each change.
ROM_EXT support
With these keymgr changes, the aforementioned OTP and flash updates, other flash updates and the UART VAL break workaround, this should be enough to boot into
ROM_EXT
Earlgrey environments.I currently have a work-in-progress, slightly hacky version of this in OpenTitan on my fork here (see the diff). I've tried running all the keymgr and sideloading test in
sival_rom_ext
and all of these are also passing, except for two tests that are marked as broken in thefpga_cw310_sival_rom_ext
environment (from a quick glance, likely because they don't make sense to run after ROM_EXT). If you have set up OpenTitan to be able to use a local QEMU override and use this branch, you can run a SiVal ROM_EXT test like:One key unresolved issue that I've hacked around in the Bazel is with
flashgen.py
, where I've had to provide the--unsafe-elf
flag because the ROM_EXT signed binary and ELF do not match. I believe this is because the ROM_EXT binary is signed together with the ROM_EXT's immutable section, which has a separate ELF file from the ROM_EXT's ELF. Hence the binary's start and size do not match the ELF. Based on this understanding, I think that the best way to resolve this would be to updateflashgen.py
with an option to provide multiple ELFs for a given binary. @rivos-eblot does this sound reasonable, or is there anything else I should be aware of here?