Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/23756 )
Change subject: security/vboot: Add boot mode selection ......................................................................
Patch Set 14:
(9 comments)
https://review.coreboot.org/#/c/23756/14/src/drivers/i2c/tpm/Makefile.inc File src/drivers/i2c/tpm/Makefile.inc:
https://review.coreboot.org/#/c/23756/14/src/drivers/i2c/tpm/Makefile.inc@5 PS14, Line 5: postcar-$(CONFIG_DRIVER_TIS_DEFAULT) += tis.c
you shouldn't replace bootblock with postcar. you should add postcar.
Done
https://review.coreboot.org/#/c/23756/14/src/drivers/spi/tpm/Makefile.inc File src/drivers/spi/tpm/Makefile.inc:
https://review.coreboot.org/#/c/23756/14/src/drivers/spi/tpm/Makefile.inc@4 PS14, Line 4: postcar-$(CONFIG_SPI_TPM) += tis.c tpm.c
need bootblock here too
Done
https://review.coreboot.org/#/c/23756/14/src/mainboard/google/poppy/Kconfig File src/mainboard/google/poppy/Kconfig:
https://review.coreboot.org/#/c/23756/14/src/mainboard/google/poppy/Kconfig@... PS14, Line 123: select VARIANT_HAS_I2C_TPM
What's this one for? What's the interaction when VBOOT_MOCK_SECDATA is enabled?
Only enable the tpm if antirollback protection isn't disabled. Does not make sense because can be set through defconfig
https://review.coreboot.org/#/c/23756/14/src/security/vboot/Makefile.inc File src/security/vboot/Makefile.inc:
https://review.coreboot.org/#/c/23756/14/src/security/vboot/Makefile.inc@75 PS14, Line 75: verstage-y += secdata_measurements.c
This should be based on a Kconfig.
Done
https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurem... File src/security/vboot/secdata_measurements.c:
https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurem... PS14, Line 21: uint32_t vboot_measure_self(void)
'self' is not really self. It's only bootblock. Sounds like you want vboot_measure_bootblock().
vboot_measure_crtm() -> core root of trust for measurement
https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurem... PS14, Line 24: PROG_UNKNOWN
Why not have PROG_BOOTBLOCK? You could just call measured_prog_run() in that case instead of hardcod […]
Done
https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurem... PS14, Line 30: if (cbfs_boot_locate(&file1, prog_name(&bootblock), NULL))
So this is assuming SEPARATE_VERSTAGE it seems?
Done
https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_tpm.c@a4... PS14, Line 448:
Where is this being done now?
vboot_logic at the beginning of verstage_main()
https://review.coreboot.org/#/c/23756/14/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/23756/14/src/security/vboot/vboot_logic.c@31... PS14, Line 315: #if IS_ENABLED(CONFIG_VBOOT_MODE_VERIFIED_AND_MEASURED_BOOT)
no #ifs
Done
-
Philipp Deppenwiese (Code Review)