Bill XIE 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 55:
(5 comments)
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... […]
Done
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
Done
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 re […]
Yes, this should never be printed, so it may be useful to catch some unlikely event in which tspi_init_crtm() is entered more than once, so I would like to retain it, but with log level changed to BIOS_WARNING.
https://review.coreboot.org/c/coreboot/+/35077/54/src/security/tpm/tspi/crtm... PS54, Line 129: "Initializing CRTM failed!");
Should probably return 0 here?
fixed as you pointed out, and log level is changed to BIOS_WARNING.
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. […]
Done