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 70:
(2 comments)
LGTM from a code perspective now, but the whitespace and line length issues still need to be fixed. If you want to fixup whitespace that's fine, but please do it in a separate CL. Do not mix feature changes with so many unrelated cleanups.
https://review.coreboot.org/#/c/29547/70/Documentation/security/vboot/measur... File Documentation/security/vboot/measured_boot.md:
https://review.coreboot.org/#/c/29547/70/Documentation/security/vboot/measur... PS70, Line 15: will be added later to the implementation. nit: I mean, it *is* possible if the code loading those FMAP partitions just calls the measuring function manually. There's just no automatic integration. But I'm not sure if it would ever make sense to add that because not all our FMAP partitions are really used in a way where that would make sense.
https://review.coreboot.org/#/c/29547/70/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/70/src/security/vboot/vboot_crtm.c@135 PS70, Line 135: pcr_index = TPM_CRTM_PCR; Actually... is this what you want? I thought the point of the whitelist was to say "this is the runtime data I want to measure (and other runtime data just shouldn't be measured at all)". But with this code, you're throwing the non-whitelisted runtime data into the code PCR instead, which seems wrong?
-
Julius Werner (Code Review)