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

[sram_ctrl, flash_ctrl, icache, otbn] Protecting scrambling keys at rest #12111

Closed
msfschaffner opened this issue Apr 14, 2022 · 11 comments
Closed
Labels
Component:Security Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones Hotlist:Security Security Opinion Needed IP:flash_ctrl IP:otbn IP:rv_core_ibex IP:sram_ctrl Priority:P3 Priority: low Triaging:MultipleBlocks Issue is relevant for the triage of multiple HW blocks Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones
Milestone

Comments

@msfschaffner
Copy link
Contributor

Scrambling keys are currently obtained from OTP and then stored locally in a register for use with the scrambling device.
However, that register is not protected against any FI injection, which may in rare cases be exploited.
Should we ECC protect these key registers and trigger an alert if there is a mismatch?

@tjaychen
Copy link

i honestly think sram_ctrl, otbn, icache by virtue of being ephemeral, it is okay for now.
otbn specifically will get software redundancy layered across.

For sram, really sensitive variables will need to be separately hardened by software, and i'm hoping if the key is attacked, it will mostly look like garbage instruction for the icache. In theory this should be difficult to reproduce across power cycles.

For flash the key is installed really early during manufacturing, so any disturbance later will actually cause code verification to fail. Any stored data will also get really wonky.

@msfschaffner could you add Bilgiday also to get his thoughts?

While i'm not saying this is bad to do, for the blocks that have passed D2S I really think we should refrain from re-opening unless we absolutely have to. The we protection I think was necessary because for ibex at least, those were unprotected otherwise. Here it is already protected, and it is just more layering.

@msfschaffner
Copy link
Contributor Author

Your reasoning makes sense. I just wanted to bring it up so we can document this decision somewhere. I will mark this as a future enhancement for now.

@msfschaffner msfschaffner added Priority:P3 Priority: low Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones and removed Priority:P2 Priority: medium labels Apr 14, 2022
@msfschaffner msfschaffner removed this from the Project: M2 milestone Apr 14, 2022
@msfschaffner msfschaffner removed the Type:Question Questions label Apr 14, 2022
@andreaskurth andreaskurth added the Triaging:MultipleBlocks Issue is relevant for the triage of multiple HW blocks label Feb 21, 2023
@andreaskurth
Copy link
Contributor

Triaged for sram_ctrl. Suggest to keep Type:FutureRelease Not relevant to currently planned releases/milestones as per the arguments above.

@GregAC
Copy link
Contributor

GregAC commented Feb 22, 2023

Triaged for flash_ctrl, happy to keep Type:FutureRelease Not relevant to currently planned releases/milestones

@GregAC GregAC added this to the Backlog milestone Feb 22, 2023
@GregAC
Copy link
Contributor

GregAC commented Feb 24, 2023

Triaged for otbn, same conclusion as above

@GregAC
Copy link
Contributor

GregAC commented Feb 24, 2023

Triaged for ibex, same conclusion as above

@msfschaffner
Copy link
Contributor Author

Do we think this is still useful to do? It seems like defense in depth and may not be warranted to spend much effort on, given that these are just ephemeral scrambling keys.

@msfschaffner
Copy link
Contributor Author

@johannheyszl @vogelpi WDYT?

@vogelpi vogelpi added the Hotlist:Security Security Opinion Needed label Aug 10, 2023
@vogelpi
Copy link
Contributor

vogelpi commented Aug 10, 2023

Johann and I think it would make sense to at least bring this up in the Sec WG. I am thus adding the corresponding label.

Personally, I think additional ECC can make sense when there is a long timing window between refreshing the scrambling key and actually using it. Otherwise, injected bit flips likely lead to ECC errors upon reading the memory with an invalid key as Tim pointed out above. Then the attacker needs to glitch the key upon loading which is harder because the timing needs to be right.

@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 6, 2023
@vogelpi
Copy link
Contributor

vogelpi commented Oct 23, 2023

We've discussed this issue in the Security Working Group meeting on Sept 14.

It was agreed that for all blocks except one, there are ECC checks further down the line and chances are high that an ECC error would be triggered fairly quickly. Extending the protection in this case would be about adding an additional layer of security. Thus, it has been decided to not change these blocks.

For OTBN, the situation is a bit different as there can be a time gap between renewing the key and filling the data memory. However, the current understanding is that we will add an algorithmic layer for protection anyway. So the additional integrity of the scrambling key wouldn't be super beneficial. Even though a potential design change could be small (don't need to change the transmission, just the storage), it may have DV implications that we would prefer to avoid at this stage. People thought it wasn't worth to unfreeze the RTL of OTBN for just this but we should keep the idea around for future releases.

I am thus closing this issue and will open a new one for OTBN exclusively. Removing the hotlist label.

@vogelpi
Copy link
Contributor

vogelpi commented Oct 23, 2023

The new OTBN-specific issues is here: #20162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:Security Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones Hotlist:Security Security Opinion Needed IP:flash_ctrl IP:otbn IP:rv_core_ibex IP:sram_ctrl Priority:P3 Priority: low Triaging:MultipleBlocks Issue is relevant for the triage of multiple HW blocks Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones
Projects
None yet
Development

No branches or pull requests

9 participants