Joel Kitching 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:
(10 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.)
https://review.coreboot.org/c/coreboot/+/35077/27/src/lib/bootblock.c@73 PS27, Line 73: !CONFIG(VBOOT) && CONFIG(TSPI_MEASURED_BOOT) Why can't we just run this unconditionally here, and remove the call from verstage?
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?
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?
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?
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?
If singular, probably don't need a blank line below.
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?
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.
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.
And for one-statement conditional bodies, we don't need the {} braces here.
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?