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/kmac] how kmac should send kmac_data_i.error #16792

Open
Tracked by #16853
weicaiyang opened this issue Dec 12, 2022 · 6 comments
Open
Tracked by #16853

[keymgr/kmac] how kmac should send kmac_data_i.error #16792

weicaiyang opened this issue Dec 12, 2022 · 6 comments
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:keymgr IP:kmac SW:cryptolib Crypto library

Comments

@weicaiyang
Copy link
Contributor

As brought up in this comment, we may want to find a better way to send the error when KMAC detects a fault during an operation requested by keymgr. We haven't had a conclusion in that discussion, so file this issue to continue on it.

Here are some approaches we brought up.

  1. current implementation (no design change needed): kmac sends error without done, and keymgr gets stuck at the operation. Eventually, the chip will be reset as kmac sends a fatal alert.
  2. make the operation complete normally: change Kmac also send done along with error. Also need to set ready high to accept the remaining data from keymgr.
  3. treat the error as an interrupt to keymgr: keymgr finishes the operation once it sees the error is set. This changes the handshaking protocol, which impacts the current DV. My assumption was that error only asserts when done is set. We need to update the block-level TB if we choose this.

As a DV person, slightly prefer to 1) or 2) as they have less DV impact. I'm open to 3) as well, if it makes more sense to the design.

cc: @ballifatih @tjaychen @eunchan @cindychip

@ballifatih
Copy link
Contributor

Thanks for the spin-off issue @weicaiyang.

I do not completely understand the alert handler, so I think I missed the part about chip reset. If a fatal error at KMAC (in particular glitching FSM) eventually forces a chip reset, then I am also fine with option 1) or 2) (in given order).

If the chip is going to reset, do we really need to care about keymgr being stuck at a state? Is it possible to have a configuration where the chip doesn't reset on KMAC fatal error?

@tjaychen
Copy link

i think that's the chip's behavior in an ideal scenario.
But it's always good to have some defense in depth here just in case the alert_handler were somehow also attacked.
I personally think number 2 would be good, but i'm fine with option 1 too if it churns things a lot.

@cdgori
Copy link

cdgori commented Dec 13, 2022

I would prefer 1 or 2 as well (mostly 2 unless we can't afford the change), as the interrupt-ish behavior of 3 means thinking about potentially a lot of other weird cases.

But if someone can make an argument for why we need 3, I'd listen.

@weicaiyang
Copy link
Contributor Author

I do not completely understand the alert handler, so I think I missed the part about chip reset. If a fatal error at KMAC (in particular glitching FSM) eventually forces a chip reset, then I am also fine with option 1) or 2) (in given order).

Once a fatal alert occurs, alert_handler will eventually reset the chip, unless something is wrong.

@ballifatih
Copy link
Contributor

Triaged for both KMAC and keymgr.

This change is not needed for M2.5 because we can deal with this issue on SW and with alert handler, as @weicaiyang suggested. Labeling this for the future milestones.

On the other hand, one thing that is critical for M2.5 is that we would like to ensure that default alert handler configuration from ROM+OTP covers this case and resets the chip when keymgr/KMAC is stuck.

In addition to this, it is best to also check on the Ibex side (when the keymgr operation e.g. advance_state is initiated) whether KMAC produces an error. This second suggestion does not look like a blocker for M2.5, as this code resides in RomExt.

cc: @bilgiday, @jadephilipoom

@ballifatih ballifatih added Triaged Type:FutureRelease Not relevant to currently planned releases/milestones Triaging:MultipleBlocks Issue is relevant for the triage of multiple HW blocks Priority:P3 Priority: low labels Feb 20, 2023
@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 7, 2023
@msfschaffner msfschaffner added Hotlist:Silicon to be resolved in Silicon Committee and removed Triaged labels Nov 8, 2023
@msfschaffner msfschaffner added Hotlist:Security Security Opinion Needed Priority:P2 Priority: medium and removed Hotlist:Silicon to be resolved in Silicon Committee Priority:P3 Priority: low labels Dec 21, 2023
@johannheyszl johannheyszl removed Priority:P2 Priority: medium Triaging:MultipleBlocks Issue is relevant for the triage of multiple HW blocks labels Jan 12, 2024
@johannheyszl johannheyszl removed the Hotlist:Security Security Opinion Needed label Jan 12, 2024
@johannheyszl johannheyszl added SW:cryptolib Crypto library and removed Type:FutureRelease Not relevant to currently planned releases/milestones labels Jan 12, 2024
@johannheyszl
Copy link
Contributor

This seems to be a ROM_ext issue, relabeled accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:keymgr IP:kmac SW:cryptolib Crypto library
Projects
None yet
Development

No branches or pull requests

7 participants