Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29547 )
Change subject: security/vboot: Add measured boot mode ......................................................................
Patch Set 60:
(11 comments)
https://review.coreboot.org/#/c/29547/60/Documentation/security/vboot/measur... File Documentation/security/vboot/measured_boot.md:
https://review.coreboot.org/#/c/29547/60/Documentation/security/vboot/measur... PS60, Line 2: extensions extension
https://review.coreboot.org/#/c/29547/60/Documentation/security/vboot/measur... PS60, Line 3: be
https://review.coreboot.org/#/c/29547/60/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/29547/60/src/security/vboot/Kconfig@38 PS60, Line 38: default "hwinfo.hex" Does it make sense to have this default here? Only Siemens mainboards have this CBFS entry.
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@34 PS60, Line 34: == 0) { Weird code formatting. Maybe change to: if (!fmap_locate_area_as_rdev("BOOTBLOCK",&bootblock_fmap)... Should fit on one line then with no need of breaking it.
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@44 PS60, Line 44: == 0) same here
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@65 PS60, Line 65: == 0) { same here
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@68 PS60, Line 68: if (tpm_measure_region(prog_rdev(&romstage), : TPM_CRTM_PCR, : CONFIG_CBFS_PREFIX : "/romstage") : != TPM_SUCCESS) { TPM_SUCCESS is defined as 0x0. Why not just: if (tpm_measure_region(prog_rdev(&romstage), TPM_CRTM_PCR, CONFIG_CBFS_PREFIX"/romstage")) return return VB2_ERROR_UNKNOWN;
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@89 PS60, Line 89: == 0) { see above
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@92 PS60, Line 92: if (tpm_measure_region(prog_rdev(&verstage), : TPM_CRTM_PCR, : CONFIG_CBFS_PREFIX : "/verstage") : != TPM_SUCCESS) { : return VB2_ERROR_UNKNOWN; here again.
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@110 PS60, Line 110: find_runtime_data_string Maybe rename this function to something like "is_runtime_data()" or "is_whitelist_data()" as it just tells you if the given file is part of the runtime/whitelist data list or not.
https://review.coreboot.org/#/c/29547/60/src/security/vboot/vboot_crtm.c@118 PS60, Line 118: + spaces around '+'
-
Werner Zeh (Code Review)