Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46432 )
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46432/3/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/3/src/security/vboot/mrc_cach... PS3, Line 92: " Missing the '\n' now.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 242: static uint32_t set_hash_space(uint32_t index, const uint8_t *data)
For complicated reasons, having an RW MRC cache hash isn't really possible for TPM 1. […]
(Still open, btw)
https://review.coreboot.org/c/coreboot/+/46432/3/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/3/src/security/vboot/secdata_... PS3, Line 167: recovery_boot nit: "recovery_boot" sounds a bit odd here (the important part is what kind of space we want to create, not how we booted, and the recovery hash space isn't necessarily created during a recovery boot). Maybe just call it "ro_space"?
https://review.coreboot.org/c/coreboot/+/46432/3/src/security/vboot/secdata_... PS3, Line 168: "MRC Hash" Might also be nice to change this... I think it might be more readable if you just use an if() rather than so many ternary operators.