Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 27:
(6 comments)
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?
Done
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. […]
¯_(ツ)_/¯ My next patch would be adding timestamp support
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.
Ack
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?
mhhhhhhh? Where is the documentation?
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: […]
Ack
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. […]
No problem. The case only exists if data is loaded more than once which is okay