lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.57k stars 772 forks source link

[ RFC, entropy_src ] Enable signal REGWEN protected #9637

Closed martin-lueker closed 2 years ago

martin-lueker commented 2 years ago

In the entropy_src design it is currently impossible to enable or disable the entropy source once the registers are REGWEN locked.

zi-v commented 2 years ago

I agree that EBABLE should not be locked. Also note that it should be possible to restart the RNG subsystem at any time, e.g., in case of FIPS health test failure.

mwbranstad commented 2 years ago

My understanding of the REGWEN function is that if you want security post hardware configuration, lock it all down. If flexibility is preferred, don't lock it. In the case of power savings, directly controlling RNG would save the most power. Entropy_src can stay enabled and locked, but will not do anything with no input, and not consume any real power.

mwbranstad commented 2 years ago

Thinking about this some more, I suppose separating the master enable from other configuration fields may be useful, and perhaps not present any security issues. By design, turning the main enable off, will act like a functional reset, exception for registers like threshold settings. From a CI passing perpective, it would be best to keep the current register structure as is. Need consensus on this one.

tjaychen commented 2 years ago

Thinking about this some more, I suppose separating the master enable from other configuration fields may be useful, and perhaps not present any security issues. By design, turning the main enable off, will act like a functional reset, exception for registers like threshold settings. From a CI passing perpective, it would be best to keep the current register structure as is. Need consensus on this one.

a bit late here. But i'm fine with this. I think we should keep the regwen in case in some use case it becomes useful. For the cases we are discussing here, we will most lkely use it as a functional reset as you say.

cdgori commented 2 years ago

Also coming to this late, sorry.

The typical security issue that comes to mind immediately is if somehow the entropy_src could be disabled but the rest of the EDN can be fooled into thinking it's capturing valid entropy (but in reality it's sampling all zeros). The health checks should be failing like crazy at that point, for one, and there ought to be some other interlocks on not sampling a disabled entropy_src. (Presumably this is the case?) @mwbranstad maybe have a think about any cases like this - I think we are pretty well covered but good to give a few minutes more thought to it.

Otherwise agree with @tjaychen - keep the regwen in case it becomes useful. If you want to break out the main enable so it can be regwen'd separately, I think that's ok.

However, it does feel a little weird to me to use the main enable as a reset - if we really need a subsystem-level soft reset, shouldn't we have such a thing as a dedicated feature rather than overloading an enable bit?

After typing all of this out, I just realized there may be a little bit of terminology skew here - do we want the ability to shut off the "noise source" (hard macro) for power savings vs the ability to shut off the "entropy_src" synthesized logic? As @mwbranstad said above, if the hard macro ("RNG") is off, the synthesized logic is not switching so the power consumption there is minimal.

I understand the desire to power down the hard macro, but then again my security concern is the same as above, make sure we can't be faked into consuming all-zeros from a disabled hard macro.

tjaychen commented 2 years ago

@cdgori so the way i view the enable... is that in a stricter system... should the entropy_src ever fail, we are just going to say "reboot, something terrible has happened".

In a more permissive system, (this is probably the point @zi-v was trying to raise), we will probably allow the software to recover without taking the whole system down. I think @mwbranstad designed the enable with the intent of being able to enable / disable without breaking. So I think in "that" sense it's kind of like a reset, in that we will go back to default state of the FSM and start over again.

zi-v commented 2 years ago

Note that due to 90B's 'Continuous health tests' requirement, every time we stop the clock (e.g. as in sleep state) or the continuous health tests from any other reason, we essentially need to run 90B's 'Start-up health tests' in order to regain 'fips' level entropy generation capability. Is it possible to do that if the 'enable' bit is locked?

@cdgori a large portion of a module consumption comes from its clock tree. So if clock is not stopped, the power save resulting from having module inactive is limited.

mwbranstad commented 2 years ago

The real point of this issue to allow more options for the system (SW) to control this module. @cdgori Tim's explanation above is what I envision as well. Turning the enable off after running for awhile allows the state of ES to "start over" when re-enabled. However, it is not a complete reset since it does not reset the register values. The original idea was that boot runs, enable is set. After boot, ES is disabled, new (final) thresholds are written in regs, then enable is set again. This issue provides a way to lock all controls EXCEPT enable for a normal use case. But of course, maybe there is a case where even the enable needs to be locked, so it is provided. @zi-v To answer your question, once the main enable is locked, it requires a hw reset to undo. This issue provides a way to run ES and be able to do a logical reset at any time.

martin-lueker commented 2 years ago

@zi-v: to clarify on Mark's point: setting enable will always go through the full 90B start sequence (and optionally add our boot sequence as well)

cdgori commented 2 years ago

@mwbranstad thanks for clarifying the "reset" - it's more like "restart the FSM" (trying to find the right phrase, but I think you understand). I think all is good then, and you should keep the current REGWEN structure but split out this enable bit to be under a separate REGWEN to allow this functionality (and the re-running of 90B on resume from sleep, which is a must).

@zi-v also understood about the clock power though I assumed, perhaps incorrectly, that clock stops were going to be ultimately done in clkmgr (or pwrmgr), not locally. I suppose this enable/disable bit could be a "vote" into the clock stop request, such that if this is disabled then clkmgr could gate the clock to ES. But maybe I don't know the actual plan there.

zi-v commented 2 years ago

Thanks all. It sounds that we are fine if we don't lock the enable. What's the use case for locking the enable? If we do, does it mean that we must keep clock & health test running constantly from one boot to the next (or until health test eventually fails)?

mwbranstad commented 2 years ago

That scenario sounds correct. So for some usages the enable should never be locked. For some usages in the future, it may be deemed that it should be locked, at least that is the idea of a separate lock.

tjaychen commented 2 years ago

yeah i think there may be very restrictive usage in the future (beyond what we have planned now).