Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 27:
(10 comments)
https://review.coreboot.org/#/c/31597/27/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/Kconfig@43 PS27, Line 43: list The semantics of this variable are not clear. It's a list of filenames but what is done with this list?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/Kconfig@51 PS27, Line 51: list ditto
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.h@63 PS27, Line 63: uint32_t What was the choice here for uint32_t return types? If it's to be consistent with other vboot return values why aren't the values commented in terms of VB2_x ?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@57 PS27, Line 57: if (fmap_locate_area_as_rdev("SI_DESC", &fmap) == 0) Why not just put these in an array with the name, string, and PCR type and loop over them?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@80 PS27, Line 80: return VB2_ERROR_UNKNOWN; There's currently no caching in fmap code. How costly is doing all these lookups?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@196 PS27, Line 196: if (!strcmp(list + i, name)) Where is the comma separate aspect dealt with in list? This looks to me that the only way possible to match is if the name is in the last part of the whitelist.
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@245 PS27, Line 245: if (!vb2_logic_executed()) Please comment as to why this check is here.
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@249 PS27, Line 249: if (!strcmp("COREBOOT", name) || We were getting flags in the fmap region metadata to indicate cbfs, right?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@257 PS27, Line 257: pcr_index = TPM_RUNTIME_DATA_PCR; Why doesn't this helper function just return the pcr index? In both cases it's the same pattern:
if (helper(...)) pcr_index = ...; else pcr_index = ...;
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@261 PS27, Line 261: snprintf(tcpa_metadata, TCPA_PCR_HASH_NAME, "FMAP: %s", name); there's no checks for remeasuring over the same region and seeing if they are the same it seems. It's just extending the pcr.