Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 14:
(6 comments)
https://review.coreboot.org/#/c/31597/14/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/#/c/31597/14/src/lib/fmap.c@83 PS14, Line 83: if (vboot_measure_fmap_hook(area, name)) I still don't really think this is a good overall approach for FMAP sections. FMAP sections aren't used like CBFS files and almost all contain runtime data anyway. You'll end up measuring many things twice, and you'll blow boot time like crazy because many FMAP sections aren't meant to be read in full on every boot. There's really no point in measuring things like vboot's NVRAM section anyway, it will probably have been updated by the time you finished booting.
I think a better approach would be to put an explicit call to measure something in the very few code pieces that access an FMAP section you'd actually want to measure.
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@32 PS14, Line 32: "SMMSTORE"}; (I also think you'll never be able to keep this list properly maintained. A lot of this can be platform-specific anyway. Another good reason to do this for individual instances only.)
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@36 PS14, Line 36: if (IS_ENABLED(CONFIG_SOC_INTEL_COMMON) || I think this should be factored out into some Intel-specific file (e.g. make this a weak function and implement it in src/soc/intel/common/crtm.c or something).
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@240 PS14, Line 240: sizeof(fmap_runtime_data[i]) I'm pretty sure this doesn't work... (FWIW strcmp() should be fine because both strings should be guaranteed to be null-terminated).
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@243 PS14, Line 243: strncpy(prefix_name, FMAP_PREFIX, TCPA_PCR_HASH_NAME - 1); snprintf(), like Patrick mentioned in the other CL too
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@252 PS14, Line 252: return 0; Oh, so the idea is that *only* the stuff in fmap_runtime_data is measured, and everything else (that isn't mentioned explicitly elsewhere) is not measured at all? Well, then that's all the more reason to just put the tpm_measure_region() call right into the code that deals with it, I think. Since the amount of regions is already enumerated anyway, it won't make a difference (and I think that's nicer than scanning through a list on every single access, and it makes it much easier to avoid duplicate measurements).