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 44:
(11 comments)
Nice, I think this is on the right track.
https://review.coreboot.org/c/coreboot/+/35077/44/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35077/44/src/arch/x86/car.ld@34 PS44, Line 34: VBOOT2_TPM_LOG If you rename the Kconfig option, maybe also rename this region?
https://review.coreboot.org/c/coreboot/+/35077/44/src/include/bootmode.h File src/include/bootmode.h:
https://review.coreboot.org/c/coreboot/+/35077/44/src/include/bootmode.h@38 PS44, Line 38: int platform_is_resuming(void); This rename touches enough files that you should put it in its own patch.
However, why do you need to move this out of vboot anyway? I don't see this touched by the core part of your patch.
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/Kconfig@1... PS44, Line 105: TSPI_MEASURED_BOOT For consistency with existing options I think TPM_MEASURED_BOOT would fit better.
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/Makefile.... File src/security/tpm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/Makefile.... PS44, Line 18: $(CONFIG_VBOOT) nit: can also just put a 'y' here
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/Makefile.... PS44, Line 50: ifneq ($(CONFIG_TPM1)$(CONFIG_TPM2),) Should be unnecessary because CONFIG_TSPI_MEASURED_BOOT already depends on these.
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi.h File src/security/tpm/tspi.h:
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi.h@56 PS44, Line 56: _entry Should this be _entries() (or just tcpa_log_replay_table())?
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/crtm... File src/security/tpm/tspi/crtm.c:
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/crtm... PS44, Line 160: void measured_boot_init_crtm(void) Does this really need to be an explicitly called function? Can't you just put something like
if (ENV_BOOTBLOCK) { static bool initialized = 0; if (!initialized) { tspi_init_crtm(); initialized = 1; } }
at the top of tspi_measure_cbfs_hook()?
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/crtm... PS44, Line 162: if (ENV_BOOTBLOCK) { This is only called from the bootblock so I think this check is superfluous?
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/tspi... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/tspi... PS44, Line 22: #if CONFIG(VBOOT_LIB) Don't conditionalize #includes. Just include these unconditionally. vboot is always checked out even if it isn't built so that should still work.
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/tspi... PS44, Line 114: return vboot_logic_executed(); There's a slight problem here in that vboot_logic.c itself extends some PCRs before it sets the vboot_executed variable to 1. But since you're already introducing a tpm_is_setup() global you can use that, so doing
if (CONFIG(VBOOT)) return vboot_logic_executed || tpm_is_setup;
should work.
https://review.coreboot.org/c/coreboot/+/35077/44/src/security/tpm/tspi/tspi... PS44, Line 309: rname, pcr, tspi_tpm_is_setup()?"measur":"logg"); nit: spaces around ternary operatory, please