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

[csrng] Add a lock CSR for en_csrng_sw_app_read #21141

Closed
vogelpi opened this issue Feb 1, 2024 · 9 comments
Closed

[csrng] Add a lock CSR for en_csrng_sw_app_read #21141

vogelpi opened this issue Feb 1, 2024 · 9 comments
Assignees
Labels
Component:RTL Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:csrng Subsystem:Entropy entropy_src, csrng, or edn related issues

Comments

@vogelpi
Copy link
Contributor

vogelpi commented Feb 1, 2024

Description

The en_csrng_sw_app_read OTP switch right now gates access to reading the internal state of all 3 CSRNG instances (SW, EDN0, EDN1) but also reading the genbits register for the SW instance, i.e., it is essential to allow using the SW instance.

Because cryptolib needs to be able to access the SW instance the switch always needs to enabled, meaning reading the internal state seems always possible which is bad. We should decouple reading the internal state and the genbits register.

Since reading the internal state is required for certification evaluation, we should convert the setting to a CSR that can be locked at runtime.

See also lowRISC/opentitan-integrated#436.

@vogelpi vogelpi added Subsystem:Entropy entropy_src, csrng, or edn related issues IP:csrng labels Feb 1, 2024
@vogelpi vogelpi added this to the Earlgrey-PROD.M3 milestone Feb 1, 2024
@vogelpi vogelpi added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Feb 1, 2024
@andreaskurth
Copy link
Contributor

@moidx: This is an open because when we have to restart CSRNG, we need to decide if we have to run a KAT based on internal state.
@vogelpi: Split access between HW and SW instance?
@moidx: For Entropy Complex 2.0, we may want to split FIPS from entropy quality. But agree that separate controls would be nice.

@andreaskurth
Copy link
Contributor

@moidx: Split per CSRNG instance

@andreaskurth andreaskurth removed this from the Earlgrey-PROD.M3 milestone Apr 26, 2024
@andreaskurth
Copy link
Contributor

andreaskurth commented Apr 26, 2024

By @vogelpi: Doesn't seem to be super important to me. Given the other work left, incl. OTBN, I suggest moving this to M4 or Backlog. We'll discuss on Monday with @johannheyszl and may move to Backlog then. In the state DB you can also read the number of reseeds, which is useful debug / performance information.

@johannheyszl
Copy link
Contributor

@vogelpi and I discussed this:

  • We think it should remain on the list, however, there are other things more critical. Also the security implication is 'only' secondary because it requires compromised SW.

  • Therefore setting as P3 in M4.

  • Since reading the internal state is useful for KATs for certification, adding a lockable CSR per instance sounds appropriate. Details of when to lock remain unclear (after restart of entropy complex, to repeat KAT, will lock be removed on clear?

  • We would suggest to only gate access to key and v using this setting but leave access to counter open.

@johannheyszl johannheyszl added this to the Earlgrey-PROD.M4 milestone Apr 29, 2024
@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 4, 2024

I've discussed this again today with @johannheyszl and we both believe we need to implement that for security reasons. The change isn't huge and I have a clear idea of how to do that. I am thus now raising priority.

vogelpi added a commit to vogelpi/opentitan that referenced this issue Jun 6, 2024
Previously, the design featured a single read enable switch plus an OTP
fuse controlling read access for all three CTR_DRBG instances. This is
non-ideal as firmware may want to perform known-answer testing of
individual instances after enabling and then lock down instances
individually.

This commit thus adds a per-instance internal state read enable on top
of the previously implemented mechanisms. After enabling, the internal
state of all instances can be read and firmware can disable read access
on a per-instance basis. Once disabled, read access remains disabled
until the module is disabled.

This resolves lowRISC#21141.

Signed-off-by: Pirmin Vogel <[email protected]>
@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 6, 2024

A PR to implement this as follows can be found here: #23539

What the PR adds:

  • Expose the reseed counter of each instance unconditionally of OTP fuses and CSRNG configuration.
  • Add a regren bit per instance that allows locking read access to the internal state (i.e. K and V) per instance. If firmware locks read access, the lock persists until the next CSRNG disablement. This is sufficient to do KAT after enabling and allows to lock internal state access moving forward.

@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 6, 2024

Alternatively, the lock could easily be made to persist until the next reset. We may also want to consider splitting the OTP fuse for internal state access and the genbits read. Without the latter, the SW context cannot be used at all. My suggestion would be to simply not factor the OTP fuse into the genbits read access. I.e., the SW context would always be usable when the SW_APP_ENABLE field in https://opentitan.org/book/hw/ip/csrng/doc/registers.html#ctrl is set. What do you think @johannheyszl @moidx ?

vogelpi added a commit to vogelpi/opentitan that referenced this issue Jun 7, 2024
Previously, the design featured a single read enable switch plus an OTP
fuse controlling read access for all three CTR_DRBG instances. This is
non-ideal as firmware may want to perform known-answer testing of
individual instances after enabling and then lock down instances
individually.

This commit thus adds a per-instance internal state read enable on top
of the previously implemented mechanisms. After enabling, the internal
state of all instances can be read and firmware can disable read access
on a per-instance basis. Once firmware is done with the known-answer
testing, it can lock the per-instance internal state read enable
settings until the next reset.

This resolves lowRISC#21141.

Signed-off-by: Pirmin Vogel <[email protected]>
@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 7, 2024

We've discussed in the triage meeting that it would be better to allow locking the read enable setting until the next reset e.g. in ROM_EXT. I've now modified the linked PR accordingly.

@vogelpi
Copy link
Contributor Author

vogelpi commented Jun 7, 2024

The OTP setting is left untouched though. The understanding is that for KATs, the internal read access is always required and that this is needed always.

@vogelpi vogelpi closed this as completed in 3a4c42f Jun 7, 2024
AlexJones0 pushed a commit to AlexJones0/opentitan that referenced this issue Jul 8, 2024
Previously, the design featured a single read enable switch plus an OTP
fuse controlling read access for all three CTR_DRBG instances. This is
non-ideal as firmware may want to perform known-answer testing of
individual instances after enabling and then lock down instances
individually.

This commit thus adds a per-instance internal state read enable on top
of the previously implemented mechanisms. After enabling, the internal
state of all instances can be read and firmware can disable read access
on a per-instance basis. Once firmware is done with the known-answer
testing, it can lock the per-instance internal state read enable
settings until the next reset.

This resolves lowRISC#21141.

Signed-off-by: Pirmin Vogel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:csrng Subsystem:Entropy entropy_src, csrng, or edn related issues
Projects
None yet
Development

No branches or pull requests

5 participants