Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29547 )
Change subject: security/vboot: Add measured boot mode ......................................................................
Patch Set 55:
(5 comments)
https://review.coreboot.org/#/c/29547/55/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/#/c/29547/55/src/lib/cbfs.c@64 PS55, Line 64: if (!ret && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT)) { Let's not spill all this logic into this function, to keep it clean. Just put
int ret = cbfs_locate(...);
if (!ret) vboot_crtm_cbfs_hook(fh, name);
return ret;
here, and then put vboot_crtm_cbfs_hook() in the header as either an empty inline or a prototype, depending on whether the feature is enabled (e.g. like timestamp_add()). You can extract the CBFS type and check for the right stage inside that function.
https://review.coreboot.org/#/c/29547/55/src/lib/cbfs.c@147 PS55, Line 147: && !IS_ENABLED(CONFIG_COMPRESS_RAMSTAGE)) There are still dozens of Jenkins errors and unrelated whitespace changes in here. It looks like you're running some sort of automated formatting tool on every file you touch... please don't do that! If you want to fix formatting somewhere, please do it in a separate CL so it's clearly separated from the new feature stuff.
https://review.coreboot.org/#/c/29547/55/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/29547/55/src/lib/prog_loaders.c@52 PS55, Line 52: if (vboot_measure_prog_hook(prog)) Same here. (Actually, why do we have two hooks now? Can't you somehow cover this from the cbfs_boot_locate() hook as well? This function calls that, after all.)
https://review.coreboot.org/#/c/29547/55/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/29547/55/src/security/vboot/vboot_crtm.h@47 PS55, Line 47: #define TPM_DATA_PCR 6 I'm honestly still a bit confused by what logic you're splitting up these PCRs. Now the "static data" may contain data that's both from the RO and RW image. What's the goal here?
Really, why do we need to use more than one PCR at all? All the information the retrace the hash is in the TCPA log, right? So wouldn't one be enough? If userland only wanted to attest some but not all components measured in there, it can just "trust" the hashes in the TCPA log for the components it doesn't care about when recreating the PCR value... that still doesn't make it any less safe for the components that it actually tries to verify. (In fact, I think for any application trying to verify this TCPA log it would probably be easier if it was all in one bucket, rather than having to follow exactly which component has been measured into where.)
PCRs are a limited (and platform-dependent) resource, and you're already wasting 6 of them now for no clear reason. I think it would be better if you just throw it all into e.g. PCR2 and leave the others available for orthogonal features.
https://review.coreboot.org/#/c/29547/55/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/55/src/security/vboot/vboot_crtm.c@209 PS55, Line 209: default: Can't you just add a CBFS_TYPE_STAGE: section here and cover the progs with that?