Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34510 )
Change subject: security/vboot: Add Support for Intel PTT ......................................................................
Patch Set 18:
(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?
Ack
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.
Ack
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. […]
I mean, these function where in secdata_* before - but i really dont care. I'll rename it tpm_common
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 cor […]
Ack
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?
Ack
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?
Ack
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.
Ack
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 […]
Ack