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 27:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35077/27/src/lib/bootblock.c File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35077/27/src/lib/bootblock.c@25 PS27, Line 25: #include <security/tpm/tspi/crtm.h>
Check your alphabetical order. (Other files too. […]
Done
https://review.coreboot.org/c/coreboot/+/35077/27/src/mainboard/siemens/mc_a... File src/mainboard/siemens/mc_apl1/variants/mc_apl2/Kconfig:
https://review.coreboot.org/c/coreboot/+/35077/27/src/mainboard/siemens/mc_a... PS27, Line 16: config VBOOT : select TSPI_MEASURED_BOOT
Shouldn't this live under TPM now?
Done in all changed variants.
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/Kconfig@1... PS27, Line 119: Runtime data whitelist of cbfs filenames. Needs to be a comma separated
This line looks longer than all the others?
Done
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... File src/security/tpm/tspi/crtm.h:
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... PS27, Line 56: *
Try to keep a blank line in between function headers?
Done
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... File src/security/tpm/tspi/crtm.c:
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... PS27, Line 25: This functions
This function or these functions? […]
Done
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... PS27, Line 162: int crtm_is_set;
Can this be a static variable inside the function?
No, crtm_is_set should be accessible with both tspi_crtm_is_set() and measured_boot_init_crtm().
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... PS27, Line 218: boot_platform_is_resuming()
If this is not just used by vboot_ anymore, we should find a better home for it.
Moved from vboot_common.h to bootmode.h as platform_is_resuming().
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/tpm/tspi/crtm... PS27, Line 223: if (result != VB2_SUCCESS) { : printk(BIOS_INFO, : "Initializing CRTM failed!"); : } else { : crtm_is_set = 1; : }
I think that printk can fit on one line now, with the longer coreboot.org line length limit. […]
Done
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/vboot/vboot_l... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/35077/27/src/security/vboot/vboot_l... PS27, Line 329: if (CONFIG(TSPI_MEASURED_BOOT) && : !(ctx->flags & VB2_CONTEXT_S3_RESUME)) { : if (tspi_init_crtm() != VB2_SUCCESS)
Can we just always run this in bootblock and remove the code here?
Done