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 45:
(11 comments)
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?
Renamed as TPM_TCPA_LOG.
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. […]
All affected spot is reverted.
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.
Done
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
Done
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.
Done
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())?
Done
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 […]
Simply doing this will cause coreboot to crash even before ehci debug becomes available, maybe because tspi_init_crtm() calls cbfs_boot_locate(), which will eventually calls tspi_measure_cbfs_hook(), forming an unwanted recursion.
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?
Done
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. […]
Done
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. […]
Done
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
Done