Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34510 )
Change subject: security/vboot: Add Support for Intel PTT ......................................................................
Patch Set 17:
(8 comments)
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/Kconfig... PS17, Line 29: comment "Anti-Rollback Protection disabled due to Intel PTT" Can we just turn this into a generic warning about MOCK_SECDATA?
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/Makefil... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/Makefil... PS17, Line 92: romstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_common.c I don't think you need this for romstage, that's only for the rec_hash stuff.
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... File src/security/vboot/secdata_common.c:
PS17: This file should not be called secdata_common.c, it should be called tpm.c or tpm_common.c or something. It contains all the TPM-related stuff that's specifically not related to secdata.
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... PS17, Line 19: #if CONFIG(TPM1) || CONFIG(TPM2) Let's do this in the header that defines the prototypes for this, that pattern is more common in coreboot (e.g. see how timestamp_get() is handled depending on Kconfig in <timestamp.h>).
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... File src/security/vboot/secdata_mock.c:
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... PS17, Line 38: #include <console/console.h> Doesn't need to be added here?
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... PS17, Line 72: uint32_t antirollback_read_space_firmware(struct vb2_context *ctx) nit: doesn't really need to move?
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... PS17, Line 425: rv = vboot_setup_tpm(ctx); This should no longer be here now.
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/vboot_l... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/vboot_l... PS17, Line 337: rv = vboot_setup_tpm(&ctx); Let's continue not checking the return value here to fit with the previous behavior and the comment above... e.g. you can write this
if (!vboot_setup_tpm(&ctx)) antirollback_read_space_firmware(&ctx);
If you want an error message, you can print it inside vboot_setup_tpm() (but I think tpm_setup() should already do that anyway?).