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 22: Code-Review+2
(5 comments)
Thanks, looks good now.
https://review.coreboot.org/c/coreboot/+/34510/17/src/security/vboot/secdata... File src/security/vboot/secdata_common.c:
PS17:
I mean, these function where in secdata_* before - but i really dont care. […]
Yes, again, we didn't used to have a distinction between TPM NVRAM stuff and other TPM stuff before, that's why we threw it all together. But if you want to add that distinction now, you'll have to separate things out correctly.
https://review.coreboot.org/c/coreboot/+/34510/22/src/security/vboot/tpm_com... File src/security/vboot/tpm_common.h:
https://review.coreboot.org/c/coreboot/+/34510/22/src/security/vboot/tpm_com... PS22, Line 23: /* Start of the root of trust */ nit: these comments really belong to the real prototypes above, not the stubs down here
https://review.coreboot.org/c/coreboot/+/34510/22/src/security/vboot/tpm_com... File src/security/vboot/tpm_common.c:
https://review.coreboot.org/c/coreboot/+/34510/22/src/security/vboot/tpm_com... PS22, Line 19: #define TPM_PCR_GBB_FLAGS_NAME "GBB flags" nit: not your patch's fault, but this should've never been called "GBB flags", that's not what it is. It should just be called "boot mode".
https://review.coreboot.org/c/coreboot/+/34510/22/src/security/vboot/tpm_com... PS22, Line 47: case BOOT_MODE_PCR: nit: Because I just had to spend another 10 minutes trying to figure out why this is written as it is... could you add a little comment here explaining
/* SHA1 of (devmode|recmode|keyblock) bits */
https://review.coreboot.org/c/coreboot/+/34510/22/src/security/vboot/tpm_com... PS22, Line 50: case HWID_DIGEST_PCR: ...and here
/* SHA256 of HWID */