Werner Zeh has posted comments on this change. ( https://review.coreboot.org/29547 )
Change subject: security/vboot: Add measured boot mode ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@33 PS3, Line 33: if (cbfs_boot_locate(&bootblock_data, : prog_name(&bootblock), : NULL) : == 0) { That formating looks weird...Either move "==0" to the prior line or use: if (!cbfs_boot_locate(&bootblock_data, prog_name(&bootblock), NULL)) {
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@41 PS3, Line 41: "bootblock" Use prog_name(&bootblock) here, too?
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@45 PS3, Line 45: fmap This is still the bootblock, right? call it bb_fmap or so for better readability?
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@52 PS3, Line 52: printk(BIOS_INFO, : "VBOOT: Couldn't measure %s into CRTM!", : "bootblock"); Do we need this printk for struct prog bootblock, too? We will have an error print friven from tpm_measure_region(), maybe this is already sufficient?
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@69 PS3, Line 69: / Why the slash here?
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@83 PS3, Line 83: if (cbfs_boot_locate(&verstage_data, prog_name(&verstage), : NULL) : == 0) { Formating again.
https://review.coreboot.org/#/c/29547/3/src/security/vboot/vboot_crtm.c@91 PS3, Line 91: / Why the slash here?