Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35077 )
Change subject: security/vboot: Decouple measured boot from verified boot ......................................................................
Patch Set 54: Code-Review+1
(7 comments)
Thanks, I think this looks good now from my side (other than a few obvious one line fixes I commented). I'll leave the final review and +2 to Philipp since this is all his code.
https://review.coreboot.org/c/coreboot/+/35077/54/src/drivers/pc80/tpm/Makef... File src/drivers/pc80/tpm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35077/54/src/drivers/pc80/tpm/Makef... PS54, Line 5: bootblock-$(CONFIG_LPC_TPM) += tis.c No longer necessary.
https://review.coreboot.org/c/coreboot/+/35077/54/src/lib/bootblock.c File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35077/54/src/lib/bootblock.c@24 PS54, Line 24: #include <security/tpm/tspi/crtm.h> No longer necessary.
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/tpm/tspi/crtm... File src/security/tpm/tspi/crtm.c:
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/tpm/tspi/crtm... PS54, Line 52: static int tcpa_log_initialized; This whole thing with a global variable and several functions seems a bit heavy-handed to me... I think you could've achieved the same by just flipping the lines in my suggestion:
if (ENV_BOOTBLOCK) { static bool initialized = 0; if (!initialized) { initialized = 1; tspi_init_crtm(); } }
But I'm fine with this too if you think it makes the code clearer.
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/tpm/tspi/crtm... PS54, Line 55: if (ENV_DECOMPRESSOR) nit: not wrong, but the decompressor can never include CBFS code anyway so also not really necessary
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/tpm/tspi/crtm... PS54, Line 73: printk(BIOS_INFO, "TSPI: CRTM already initialized!\n"); I don't see why you need this? This should never be printed, right? (Or maybe it does during that recursion thing you mentioned, but then I'm not sure why you'd always want this message to be printed.)
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/tpm/tspi/crtm... PS54, Line 129: "Initializing CRTM failed!"); Should probably return 0 here?
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/vboot/symbols... File src/security/vboot/symbols.h:
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/vboot/symbols... PS54, Line 23: DECLARE_REGION(tpm_tcpa_log) nit: move this to <symbols.h>? (Not really sure why this separate file still exists anyway, we've been throwing all kinds of optional feature or arch specific stuff into <symbols.h> lately.)