Thanks, I like this a lot better now! A few more nits and optional suggestions but I'm generally okay with this now.
Patch set 35:Code-Review +1
10 comments:
File Documentation/security/vboot/measured_boot.md:
Patch Set #35, Line 35: * All non CBFS partitions are measured on lookup.
Rewrite to match the implementation you have now?
File src/drivers/intel/fsp1_0/fastboot_cache.c:
Patch Set #35, 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>
File src/drivers/mrc_cache/mrc_cache.c:
Patch Set #35, 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?
File src/drivers/smmstore/store.c:
Patch Set #35, Line 103: if (vboot_measure_fmap(&store, CONFIG_SMMSTORE_REGION))
What about CONFIG_SMMSTORE_IN_CBFS?
File src/security/vboot/vboot_crtm.c:
Patch Set #35, 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?
Patch Set #35, 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?
Patch Set #35, Line 71: "FMAP: %s", ifd_static[i]);
Why not just call vboot_measure_fmap() rather than implement this (twice)?
Patch Set #35, 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)?
Patch Set #35, 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).
Patch Set #35, 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.
To view, visit change 31597. To unsubscribe, or for help writing mail filters, visit settings.