EDN D2S Review AIs

[mwbranstad]

• [msf] talk to tim about burying MUBI/LC signals, maybe add sec_buf to the sync?
• [mark] use prim_mubiX_sync (flopless) to create multiple copies and consume the MUBI at the endpoints with multiple decoding functions (applies to CSRNG, entropy_src, EDN) -> [csrng/rtl] d2s review updates #10132, [edn/rtl] D2S review updates #10448
• [mark] consider describing in the Hjson description which FSMs are protected. -> [edn/rtl] D2S review updates #10448
• [mark] rename LOGIC.INTEGRITY to something like ENTROPY_IN.BUS.CONSISTENCY to make it more specific -> [entropy complex/security] countermeasure lists and labels updated #10118
• Pending comment fix related to this CM: [edn] update comment #10032
• [mark] add security note to documentation for the user to do a 32bit consistency check since EDN only does this at the 64bit level. Note that this may only be needed for high-quality entropy consumers. E.g. only if FIPS bit is being checked. -> also make an issue for OTBN, keymgr, tag Tim and OTBN guys. -> [edn/rtl] D2S review updates #10448
• [mark] Regarding boot request mode: add security note to documentation. This is not important if we do not care about the FIPS bit, but if we do care about the FIPS bit, consumers need to pull entropy until the FIPS bit is set to 1 to make sure the quality is guaranteed. -> also make an issue for OTBN, keymgr, tag Tim and OTBN guys. -> [edn/rtl] D2S review updates #10448
• [mark] Need to align standardized names with terminology -> [entropy complex/security] countermeasure lists and labels updated #10118
• [chris] Need to add a countermeasure to the spreadsheet for any Mubi used, EDN.CTRL.MUBI
• [mark] Add EDN.CONFIG.REGWEN -> [entropy complex/security] countermeasure lists and labels updated #10118
• [chris] Update spreadsheet
• [mark] documentation says es_fifo_err interrupt cause is (push & full) | (pop & empty) - it’s missing (full & empty) condition from the RTL -> [edn/rtl] D2S review updates #10448

[tjaychen]

which mubi signals did you guys have in mind? If it's the lc sync ones those are already covered.
If it's the software configuration bits, I don't really think there's a need.
If it's the OTP allow read bits I also don't think there is a need since that will be double gated by a required software exploit.

Good point on adding the note for the security notes, but I think for the FIPS stuff, I'm pretty sure we determined LONG ago all the masking modules can live without it. And OTBN already has split random bus (with the implication that the high quality ones must be FIPS).

[msfschaffner]

Oh so for the MUBI item above I just wanted to double check whether the prim_mubi*_sync and prim_lc_sync modules have sec buffers to create the buffered copies.

If they do, @mwbranstad could use a combinational version (no sync flop) of prim_mubi*_sync to create multiple MUBI copies and consume them in several places using the decode function.
This should obviate the need to do any additional manual buffering of a single decoded bit.

As it looks right now, I believe prim_lc_sync uses sec buffers:

opentitan/hw/ip/prim/rtl/prim_lc_sync.sv

Lines 60 to 64 in 88db0fb

	prim_sec_anchor_buf u_prim_buf (
	.in_i(lc_en[k]),
	.out_o(lc_en_out[k])
	);
	end

But prim_mubi*_sync does not:

opentitan/hw/ip/prim/rtl/prim_mubi4_sync.sv

Lines 118 to 121 in 88db0fb

	prim_buf u_prim_buf (
	.in_i(mubi[k]),
	.out_o(mubi_out[k])
	);

Should we align this?

[tjaychen]

i actually purposefully left sec_anchor_buf outside of mubi_sync. I don't think all mubi_sync qualify as critical nets we have to bury. So if we go down that route I think that would be too many. The life cycle broadcast signals are a category of their own, that's why I buried them into prim_lc_sync directly.

We can make a reasonable argument that all mubi should fall into this category, but I was a bit concerned it would become too many and should be dealt with on a case by case basis.

[msfschaffner]

Yeah that is a good point and the occurences within CSRNG probably fall into this category as you mentioned above.

That being said, I think there may still be value in using prim_mubi*_sync to create multiple copies if there are multiple endpoints, in order to ensure these all use separate decoders. WDYT?

[tjaychen]

definitely agree. I actually thought that's what we were using them for. I think that's the reason you had the `Async` options on them right? So we can choose to skip the synchronizers and just deploy a buffer.

…

On Fri, Jan 21, 2022 at 7:05 PM Michael Schaffner ***@***.***> wrote: Yeah that is a good point and the occurences within CSRNG probably fall into this category as you mentioned above. That being said, I think there may still be value in using prim_mubi*_sync to create multiple copies if there are multiple endpoints, in order to ensure these all use separate decoders. WDYT? — Reply to this email directly, view it on GitHub <#10262 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AAH2RSVSY3TFLXJYRNNKB6DUXINGDANCNFSM5MQTXLWQ> . 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>. You are receiving this because you were assigned.Message ID: ***@***.***>

[msfschaffner]

Indeed, that is the reason.

@mwbranstad, this means that you can go ahead with the solution we had discussed in the meeting today that uses the prim_mubi*_sync (with async parameter set to 0) to generate copies of the MUBI and then consume the copies at the individual endpoints using individual decode function calls.

[mwbranstad]

Thanks @msfschaffner and @tjaychen for precise clarification on this point.

[mwbranstad]

@cdgori chris, can you confirm the last two checklist items, then check them off?

[cdgori]

@cdgori chris, can you confirm the last two checklist items, then check them off?

I added the missing countermeasure and aligned the entire spreadsheet with the HJSON, so those 2 tasks are complete.

I can't edit your comment to check the boxes though (?)

[mwbranstad]

@msfschaffner all the boxes are checked now. Should we close this issue, or are there some additional steps to do before that happens in the security review flow?

[msfschaffner]

@mwbranstad same comment as on #10096 (comment), number 3). Also, some of the links above seem to be broken.

[mwbranstad]

Fixed broken links.

[msfschaffner]

Thanks for fixing them @mwbranstad. Just noting here that it seems interesting to see several mentions of #10132 which does not touch any of the EDN files... did you perchance paste the wrong reference?

[mwbranstad]

yes, the links were indeed wrong, and have now been corrected.

[msfschaffner]

Thanks for correcting them.
It looks like we've addressed all EDN items then (waiting to close this via the D2S PR).