Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29547 )
Change subject: security/vboot: Add measured boot mode ......................................................................
Patch Set 47:
(7 comments)
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.h@23 PS47, Line 23: #if IS_ENABLED(CONFIG_CHROMEOS) This isn't helping... PCRs 0 and 1 are extended by vboot itself, regardless of whether you're using CONFIG_CHROMEOS. You need to make this branch the default for everyone. (That seems way less confusing than having two possible configs anyway.)
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.h@69 PS47, Line 69: // e.g. CMOS, NVRAM... nit: I'm not sure if we really said we'd allow C99 comments in coreboot now, but even if so probably best to not mix the two within the same file.
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.h@70 PS47, Line 70: #define TPM_WHITELISTED_PCR 23 nit: Is there a good reason these are all so far apart? Why not just use RO = 8, RW = 9, payload = 10, unknown = 11 and whitelisted = 12 or something like that?
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.c@104 PS47, Line 104: } nit: You essentially have this part three times now, so it probably makes sense to factor it out into a measure_stage(cbfs_path, pcr) function.
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.c@116 PS47, Line 116: if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)) Rather than doing this, I think it would be cleaner to put a
if (!vboot_logic_executed()) return 0;
at the top of this file (otherwise, you have the same problem with a separate verstage).
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_crtm.c@142 PS47, Line 142: return -1; Should this print some error as well?
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/29547/47/src/security/vboot/vboot_logic.c@31... PS47, Line 319: | I assume this is supposed to be an & ?
(Also, maybe this would thematically fit better in extend_pcrs()?)
-
Julius Werner (Code Review)