Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 35: Code-Review+1
(10 comments)
Thanks, I like this a lot better now! A few more nits and optional suggestions but I'm generally okay with this now.
https://review.coreboot.org/c/coreboot/+/31597/35/Documentation/security/vbo... File Documentation/security/vboot/measured_boot.md:
https://review.coreboot.org/c/coreboot/+/31597/35/Documentation/security/vbo... PS35, Line 35: * All non CBFS partitions are measured on lookup. Rewrite to match the implementation you have now?
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/intel/fsp1_0/f... File src/drivers/intel/fsp1_0/fastboot_cache.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/intel/fsp1_0/f... PS35, Line 64: if (vboot_measure_fmap(&rdev, "RW_MRC_CACHE")) { nit: could also just do
if (!vboot...) { <stuff right below here> } <fall through to other error case below>
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/mrc_cache/mrc_... PS35, Line 316: if (vboot_measure_fmap(rdev, cr->name)) nit: I am not an expert on this code, but I think(?) the MRC_CACHE region is made up of a chain of records, and only the latest record counts (that's what mrc_cache_latest() finds). So are you sure you want to measure the whole region, and not just the stuff you're using?
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/smmstore/store... PS35, Line 103: if (vboot_measure_fmap(&store, CONFIG_SMMSTORE_REGION)) What about CONFIG_SMMSTORE_IN_CBFS?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 55: if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT_IFD)) { Can't you just put this function in soc/intel/common (or maybe cpu/intel) somehow, rather than needing the Kconfig?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { Are you okay to silently ignore this if the region isn't found?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 71: "FMAP: %s", ifd_static[i]); Why not just call vboot_measure_fmap() rather than implement this (twice)?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 200: if (!whitelist_len || !name_len) nit: check is probably unnecessary now, at least the first part (and then you don't need to strlen() the whitelist)?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 206: memcmp This should be a strncmp() to make sure you don't overrun the whitelist (because memcmp() is allowed to compare more than one byte at a time).
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 253: if (!vboot_logic_executed()) nit: I'd consider making this an assert() instead. Since this is not a hook but called explicitly, you really never want anything to call it before verstage, and if anything does you'll want to notice.