[reggen] Add tooling for checking in-line countermeasure annotations

[msfschaffner]

As discussed in the entropy_src D2S review meeting today, it would be great if we could annotate the code with the countermeasure IDs.

These would not be exact annotations, but rather serve as hints where an auditor should look in order to audit a specific countermeasure. I.e., certain countermeasures can be pinpointed exactly - think prim like instances like prim_count, whereas algorithmic countermeasures like masking schemes are a bit less well localized.

The tentative format for such annotations should be // SEC_CM [INSTANCE].ASSET.CM_TYPE in order to facilitate checks in reggen.

In particular, reggen should:

• Extract all lines starting with // SEC_CM from ipname/rtl/*.sv files
• Match them up against the Hjson list defined, and throw an error if
  • the countermeasure annotation does not exist in the Hjson list
  • a countermeasure from the Hjson list cannot be matched up with an RTL annotation (it is allowed to use the same countermeasure ID multiple times in RTL)

Possible future enhancements:

• Instead of just grepping files under ipname/rtl/*.sv, fusesoc could be invoked to build the file list
• In the documentation, we can append a list of links to each countermeasure ID that takes the reviewer/auditor to the code section with the annotation (on GitHub).

[msfschaffner]

CC @sriyerg

[tjaychen]

that sounds good.
so hypothetically speaking, let's say there's something called CTRL.MUBI, but in the design there are actually multiple instances of such.

Each one would then probably match to the same entry in the hjson?

[msfschaffner]

That is correct, unless we want to enforce a 1:1 correspondence.
Note that such 1:1 correspondence would then probably have to be checked on a per SV module basis, since reggen cannot elaborate generate loops that create multiple instances of the same module.
Do we have any preference?

[tjaychen]

i dont think so. I think 1 to many should be fine, especially when it comes to the more standard types.
If during the review process we find the need to split them up, then it might be up to the designer to actually tag them with separate prefixes so they can be separately identified.

[msfschaffner]

Yes, I think I am also leaning towards leaving more flexibility.
For the standardized ones, this would actually allow us to collapse them into one item, whereas for more custom stuff we probably want to call them out specifically.

This would eliminate quite some replication in blocks like OTP:

opentitan/hw/ip/otp_ctrl/data/otp_ctrl.hjson

Lines 892 to 909 in 7df0097

	{ name: "DAI.FSM.SPARSE",
	desc: "The direct access interface FSM is sparsely encoded."
	}
	{ name: "KDI.FSM.SPARSE",
	desc: "The key derivation interface FSM is sparsely encoded."
	}
	{ name: "LCI.FSM.SPARSE",
	desc: "The life cycle interface FSM is sparsely encoded."
	}
	{ name: "PART.FSM.SPARSE",
	desc: "The partition FSMs are sparsely encoded."
	}
	{ name: "SCRMBL.FSM.SPARSE",
	desc: "The scramble datapath FSM is sparsely encoded."
	}
	{ name: "TIMER.FSM.SPARSE",
	desc: "The background check timer FSM is sparsely encoded."
	}

[vogelpi]

I definitely favor flexibility, too. Collapsing multiple standardized CM instances seems more acceptable with the annotations in the RTL. However, there may still be cases if the list of CM for a particular instance are different in which case giving individual names simplifies things . For example, you may have different FSMs in an IP that all use the same base hardening but only one FSM features global escalation, too.

[mwbranstad]

Initial thought was that 1:1 would be more effective, but after more consideration, maybe just having the broad CM listed in the hjson and the instance name in the RTL is sufficient.

[msfschaffner]

@mwbranstad since we would like to be able to globally aggregate and list CM IDs, I think we should not use different IDs in RTL annotation for consistency.

I.e., either the annotator chooses to make instance names explicit, and then the list in Hjson should reflect that, or they choose to use the general ID, in which case you get multiple tags of the same name in RTL.

[sriyerg]

+1 for flexibility - it will be useful to enforce for example, ALL instances of some critical counter measure foo are properly verified, once we start extracting testplan bits from these.

[cdgori]

As the reviewer, probably 1:1 would be easier to deal with. But that basically makes the countermeasure lists unwieldy and forces a need for manual reconciliation which is not desirable. So 1:many for things like MUBI is ok for me, I think. We'll check the individual MUBIs in D2S and have to keep in mind that downstream changes to anything countermeasure-related (adding or removing) probably mean retriggering a D2S review.

[cdgori]

One other comment, what I am finding in the review process is that we are pursuing two goals in parallel:

1. for annotated/stated countermeasures, are they present / properly used?
2. are any countermeasures missing that should be present - this usually by enumerating assets and trying to think about attacks / potential design weaknesses or single points of failure.

The second one is harder without fully enumerated 1:1 type of lists. I think the flexibility is still probably worth it, it just means that we spend a little more time on the review (a one-time cost, hopefully) to get everything accounted for.

[msfschaffner]

Thanks for all the feedback here. We now have the tooling in place that can check

1. whether the canonical names are correct
2. whether all items annotated in RTL are actually listed in Hjson
3. whether all items in Hjson are annotated in RTL

Note that 2) produces an error and 3) a warning at the moment since we still have some un-annotated blocks that already have an Hjson list merged. That warning should be bumped to an error once all blocks have been annotated.

As for additional enhancements, one thing that I would find very neat would be to use the extracted info to enhance the autogen'd table and add links to the individual label instaces in the code like this (example from sram_ctrl):

Countermeasure ID	Description	RTL Annotation
SRAM_CTRL.BUS.INTEGRITY	End-to-end bus integrity scheme.	sram_ctrl.sv:96 sram_ctrl.sv:392
SRAM_CTRL.MEM.INTEGRITY	End-to-end data/memory integrity scheme.	sram_ctrl.sv:378
SRAM_CTRL.MEM.SCRAMBLE	Data is scrambled with a keyed reduced-round PRINCE cipher in CTR mode.	sram_ctrl.sv:408
SRAM_CTRL.ADDR.SCRAMBLE	Address is scrambled with a keyed lightweight permutation/diffusion function.	sram_ctrl.sv:408

I don't think it is hard to do, given that we already extract this info as part of the RTL annotation checking. We would just need to append this info to the datastructure stored in the IpBlock object, and expand the rendering step here:

opentitan/util/reggen/gen_cfg_html.py

Lines 116 to 128 in dc40cb5

	if not cfgs.countermeasures:
	genout(outfile, "<p><i>Security Countermeasures: none</i></p>\n")
	else:
	genout(outfile, "<p><i>Security Countermeasures:</i></p>\n")
	genout(
	outfile, "<table class=\"cfgtable\"><tr><th>Countermeasure ID</th>" +
	"<th>Description</th></tr>\n")
	for cm in cfgs.countermeasures:
	genout(outfile,
	'<tr><td>{}</td>{}</tr>'
	.format(cfgs.name.upper() + '.' + str(cm),
	render_td(cm.desc, rnames, None)))
	genout(outfile, "</table>\n")

If anyone feels inclined to take a stab at this please go ahead. Otherwise I'll come back to this enhancement once I find some spare cycles in my backyard.

[tjaychen]

btw i think one thing we "might" not be dealing with yet is the templated modules. I've run into a few cases where either the wrong hjson or maybe the wrong rtl was being inspected? do you happen to know?

[msfschaffner]

Ah, this is something we need to double check.

The regtool does the check on the files that live at ../rtl/ relative to the hjson, so it could be that this currently only works for generated IPs that are generated with IP gen, since that one copies the entire documentation over as well (and the docs are built from the generated folder)...

Could this be the issue?

[matutem]

It turns out ipgen is not required: what needs to be done is to avoid running regtool in hw/Makefile for generated blocks, since topgen will run regtool with the right hjson file. This is addressed by #21053.

There is also an issue (#20887) to add missing countermeasure annotation to some blocks which issue warnings.

Once these two items are dealt with we could turn missing annotations into errors by default, perhaps with a flag to turn them into warnings for RTL development?

[vogelpi]

@matutem , I've assigned you to this issue because it is my understanding that you are in the process of finishing the last items before we can switch the warnings into errors. Please let me know if this is not correct, thanks!

[andreaskurth]

Matute has a PR that converts flash_ctrl to ipgen, and a follow-up PR will resolve this

[matutem]

An update: #22848 has a change that triggers errors for any unimplemented countermeasures. Unfortunately, it is failing for pinmux because we invoke regtool.py against hw/top_earlgrey/ip/pinmux/data/autogen/pinmx.hjson, and that fails. The reason is that it cannot find all the sources, since they are split between hw/ip/pinmux and hw/top_earlgrey/ip/pinmux (for autogen code). It could be messy to implement a fix in regtool, and this problem won't affect pinmux if it was generated by ipgen.

I would like to commit this fix once pinmux is done via ipgen, which it should be done as part of #8440.

[andreaskurth]

Discussed in triage meeting, agreed to continue tracking this in M4