opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

[keymgr] Verify entropy from EDN is used correctly

Open weicaiyang opened this issue 4 years ago • 9 comments

@tjaychen

As we discussed, it's good to prove EDN data are used to wipe the secrets in keymgr.

EDN -> LFSR -> used to wipe secret/data output

From EDN to the input of LFSR, we can check EDN data are sent to LSFR as seeds. But after LFSR, the value is scrambled. Sometimes, it's constantly scrambled for a period of time. Hence it's hard for DV to predict the correct outcome.

weicaiyang avatar Sep 30 '21 20:09 weicaiyang

o you mean inside prim_lfsr the scrambling that's done?

On Thu, Sep 30, 2021 at 1:54 PM weicaiyang @.***> wrote:

@tjaychen https://github.com/tjaychen

As we discussed, it's good to prove EDN data are used to wipe the secrets in keymgr.

EDN -> LFSR -> used to wipe secret/data output

From EDN to the input of LFSR, we can check EDN data are sent to LSFR as seeds. But after LFSR, the value is scrambled. Sometimes, it's constantly scrambled for a period of time. Hence it's hard for DV to predict the correct outcome.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/8458, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RSWMPOFVOA4OS5TOYGDUETE7PANCNFSM5FDGPRSA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

tjaychen avatar Sep 30 '21 20:09 tjaychen

o you mean inside prim_lfsr the scrambling that's done?

yeah, need to know how the EDN input is scrambled by prim_lfsr and then used to wipe data. In invalid state, SW output and sideload output are wiped by random data from LFSR. If I can predict these random data based on the EDN input, then I can do end to end comparison.

weicaiyang avatar Sep 30 '21 21:09 weicaiyang

@weicaiyang i feel like this one is either nice to have / backlog at this point... i feel like trying to model how the lfsr scrambles in dv is probably not worth it at this point... let me know what you think.

tjaychen avatar Jan 13 '23 21:01 tjaychen

@weicaiyang i feel like this one is either nice to have / backlog at this point... i feel like trying to model how the lfsr scrambles in dv is probably not worth it at this point... let me know what you think.

agree, I still can't find a good way to verify this. Changed to backlog.

weicaiyang avatar Jan 13 '23 21:01 weicaiyang

Hey @ballifatih , I think that based on the description this PR actually matters for keymgr. I've now added the corresponding label and updated the title.

vogelpi avatar Feb 24 '23 16:02 vogelpi

Thanks @vogelpi, good catch!

Triaged for keymgr. I think we should include this in M2.5.

It might be a bit challenging to get the exact LFSR state that is written onto the cleared key, because when the clear mode is activated, it continuously wipes the key (see the thread). Because we toggle the clearing on and off from SW side, I do not think that the number of clock cycles is fixed.

If we cannot do this ideal test, we should probably at least test that the initial seed loaded into keymgr's LFSR is correct.

ballifatih avatar Feb 24 '23 18:02 ballifatih

Yes, I agree with you @ballifatih . It's probably not worth to predict the correct LFSR state/output. What we could do the following with SVAs in the design:

  1. Verify that EDN input is indeed loaded into LFSR state.
  2. Verify that after wiping, the key register indeed changes to a different value / the LFSR output. Both this is already done for AES, too. I can give you a pointer if needed. Just let me know when you start working on this.

vogelpi avatar Feb 27 '23 09:02 vogelpi

When I try to replicate the SVAs from AES, I see two irregularities for keymgr:

  1. Unlike AES, where there are multiple independent 32-bit LFSR chunks, keymgr consist of single 64-bit LFSR. Two 32-bit reads are combined with prim_packer_fifo inside prim_edn_req and then 64-bit is written to LFSR. Note that, LfsrWidth = 64: https://github.com/lowRISC/opentitan/blob/060354c410761db905f385909c2b163b0076258d/hw/ip/keymgr/rtl/keymgr_reseed_ctrl.sv#L60-L75
  2. When there is an ongoing operation in keymgr, LFSR update operation is ignored. This means if we write an SVA with the assumption that every valid prim_edn_req output is written to LFSR, we would see some violations. https://github.com/lowRISC/opentitan/blob/060354c410761db905f385909c2b163b0076258d/hw/ip/keymgr/rtl/keymgr.sv#L215-L220

What I can think of is to provide partial coverage with SVAs that capture:

  1. Ensure that what comes out of prim_edn_req (64-bit) is written to LFSR.
  2. Ensure that every even 32-bit read from EDN comes out as one half of prim_edn_req
  3. When there is key wipe, ensure that key is updated with something new (I am guessing it would be difficult to guess which state LFSR evolves to at that point)

WDYT @vogelpi ?

ballifatih avatar Mar 10 '23 14:03 ballifatih

estimate 32

ballifatih avatar Mar 24 '23 14:03 ballifatih

@vogelpi to take a look again.

vogelpi avatar Jun 14 '24 15:06 vogelpi

I think we should try to include this for M5. The strategy proposed by @ballifatih above sounds good to me (sorry for not getting back to you Fatih, I completely forgot about this one obviously).

Would this be something you could help with @ballifatih for M5?

vogelpi avatar Jun 18 '24 22:06 vogelpi

For M5 sign-off we are planning to follow an RTL review by inspecting the waveforms.

@ballifatih will take a look at the waveforms.

@vogelpi, someone else should help with implementing the assertions. It is probably ok to move the assertions to M7 and rely on the RTL/waveform auditing for M5.

moidx avatar Jun 24 '24 14:06 moidx

I have done waveform review by slightly modifying keymgr_sideload_kmac test. I have not encountered any issues during my review.

Regarding reseeding mechanism:

  • [x] Initial seeding request happens after the first advance call.
  • [x] Further reseed requests happen when the counter hits the configured threshold. I triggered this by continuously clearing sideload slots.
  • [x] In both cases, I was able to verify that the randomness coming from END ports end up at the output of keymgr_reseed_ctrl.
  • [x] Each reseeding consists of two consecutive 32b EDN requests. Each request is acknowledged with 4 clock cycle delay.

Inside Keymgr, reseeding logic feeds a single 64-bit LFSR, and its output is used for three main purposes:

  • For ctrl logic:

    • [x] random_req -> During keymgr init, used to fetch few rounds of entropy to populate initial randomness to key state.
    • [ ] If a fault occurs, then the randomness is used to overwrite the key state. I could not verify this because I was not able to trigger fault in the test.
    • [ ] If Keymgr moves to disabled/invalid, then it forces 2 reseed operations before moving to that state. I was not able to verify this because the test ends without Keymgr moving to disabled/invalid.
  • For keymgr KMAC interface:

    • [x] Whenever a 64b message block is sent to KMAC, a decoy 384b value is re-constructed as a safe value to be propagated to the internals of Keymgr.
  • For sideload ports:

    • [x] When clearing is active, used to continuously clear sideload key ports.

In few cases, it was not possible to see the same randomness value being transmitted correctly due to two challenges:

  • Whenever 64-bit LFSR output is used, this 64-bit value is first divided into two 32b chunks. Then, 64b consumer (such as ctrl logic) actually uses only one of the 32b halves, and computes the other half as a function of this 32b value. I was therefore only see the half of the value coming from the LFSR.
  • When LFSR is reseeded, it is not possible to see the seed value as the LFSR output, because of its non-linear additional layer. However, I was able to verify that the correct value is latched internally.

ballifatih avatar Jun 25 '24 20:06 ballifatih

Thanks for the doing the inspection and reporting back the results @ballifatih , this is perfect!

I am now moving this to M7 for adding the SVAs as proposed by @moidx . I've increased the effort spent by 0.5 to record your work Faith.

vogelpi avatar Jun 27 '24 12:06 vogelpi