Skip to content
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

[keymgr] D2(S) Signoff #20981

Closed
msfschaffner opened this issue Jan 25, 2024 · 4 comments
Closed

[keymgr] D2(S) Signoff #20981

msfschaffner opened this issue Jan 25, 2024 · 4 comments

Comments

@msfschaffner
Copy link
Contributor

msfschaffner commented Jan 25, 2024

Description

Ensure D2(S) signoff criteria are still maintained (this is not a focus area block).

@msfschaffner msfschaffner added this to the Earlgrey-PROD.M2 milestone Jan 25, 2024
@msfschaffner msfschaffner changed the title [keymgr] D2 Signoff [keymgr] D2(S) Signoff Jan 25, 2024
@andreaskurth
Copy link
Contributor

andreaskurth commented Mar 26, 2024

Commits since Earlgrey-ES tapeout

git rev-parse HEAD

c73929e

git log Earlgrey-M2.5.2-RC0..HEAD --oneline hw/ip/keymgr
  • 52da8f7 [keymgr] Add missing CM annotation
    --> RTL changed but only a CM label added
  • 0d14dd6 [docs] Fix repetitions of the definite article
    --> no RTL changed; spelling fixed in spec
  • 5639924 Revert "[edn] Move prim_edn_req out of prim"
    --> reverts 3b4e36e below
  • c721c51 [rtl, prim] Add 'commit' functionality to prim_count
    --> RTL changed but no functional difference
  • e81b588 [keymgr/otp_ctrl] Add support for creator/owner seeds
    --> RTL changes with minimal functional impact for keymgr:
    1. In otp_ctrl_pkg::otp_keymgr_key_t the creator root key shares got renamed and the valid signal got split into one valid signal per share.
    2. keymgr can now take the creator and owner seed from flash instead of OTP iff the UseOtpSeedsInsteadOfFlash synthesis parameter is enabled, which is disabled in Earlgrey -- so this does not change functionality for Earlgrey.
  • 61a237e [util/reggen] reverse order of substruct generation
    --> RTL changed but no functional difference
  • 3b4e36e [edn] Move prim_edn_req out of prim
    --> reverted by 5639924 above
  • de31bdf [reggen] Remove the devmode input
    --> RTL changed but no functional difference
  • 963a500 [doc] Minor tweak to md sanitisation code
    --> no RTL changed; spec docs changed but changes should be structural only
  • 975a6eb [adc_ctrl,dv] Tidy up access to intr_state in env_cfg files
    --> no RTL nor specification changed
  • 1b16ca2 [reggen] Add mubi support SWAccess that sets/clears a reg
    --> RTL changed but no functional difference
  • 59f8142 [doc] Moved badges over to using hosted images
    --> no RTL nor specification changed
  • 8ba7e45 [SiVal] Test plan updates for keymgr
    --> no RTL nor specification changed
  • 19c05ad [doc] keymgr registers and interfaces now use CMDGEN
    --> no RTL changed; spec docs changed but changes should be structural only
  • 7688e71 [reggen] Add initial support for version and cip_id hjson fields
    --> no RTL changed
  • fbd888e Revert "[reggen] Add CIP_IDs and bump all major versions"
    --> reverts commit below
  • 0ba10b3 [reggen] Add CIP_IDs and bump all major versions
    --> reverted by commit above
  • e47df29 [misc] Use lc_tx_t testing functions at endpoints
    --> RTL changed but no functional difference

Issues closed since the Earlgrey-ES tapeout

https://github.com/lowRISC/opentitan/issues?q=is%3Aissue+is%3Aclosed+closed%3A%3E2023-06-27+keymgr

Currently open issues

https://github.com/lowRISC/opentitan/issues?q=is%3Aissue+is%3Aopen+keymgr

Summary

Re RTL changes, e81b588 is the only RTL change with minimal functional impact; the only functional change is that each creator root key share now has its own valid signal. The two valid signals get ANDed in keymgr_ctrl before u_key_valid_sync. This doesn't create glitch problems because the signals only ever transition from 0 to 1. DV has been changed to take this into account.

Re closed design issues since Earlgrey-ES tapeout, #19146 fixed a bug in prim_subreg_arb (#5146), which is relevant for keymgr, and #21372 changed lc_ctrl so that keymgr now gets a unique diversification value for each group of life cycle states (resolving #14047). Top-level tests cover these changes.

Re open design issues, #8120 is an open item for D2S, so we should not sign off D2S at this point. #22117, #22296, and #22297 could be open items for D2 but they are under discussion, so I suggest we don't gate this signoff on them. If we decide that at least one of those items need a keymgr design change, that could be a minor change as part of D3 or, if it's a major change, we will have to re-open D2.

In conclusion, I think this analysis supports signing keymgr off at D2 but not D2S. I'll create a separate issue for keymgr D2S signoff as part of M3.

@andreaskurth
Copy link
Contributor

@vogelpi: May I ask you for feedback on my analysis and D2 signoff approval if you agree?

@vogelpi
Copy link
Contributor

vogelpi commented Mar 28, 2024

Thanks for putting this together @andreaskurth !

To be honest, I am in favor of signing of D2S. This is the state keymgr was before and as you pointed out, no significant changes have gone in since the last sign-off. So there is IMO no need to go back down again. I agree that there are a couple of security hardening changes we want to do for M3. But all of them are minor. We also keep adding security improvements to other blocks after hitting D2S.

D2S just says security countermeasures are implemented and this is the case for keymgr. The things coming in M3 are not about adding new countermeasures but minor improvements to existing things.

So to summarize: signing of at D2 is definitely okay, signing of at D2S would be preferred from my side. It will save us further paper work in M3.

@andreaskurth
Copy link
Contributor

Alright, given that (1) keymgr has been signed off at D2S before and the changes since then don't revert D2S and (2) #8120 is tracked for M3, I agree that we can sign keymgr off at D2S again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants