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 4:
(3 comments)
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)
Done in https://review.coreboot. […]
Hmmm... okay so we're just completely forbidding it for TPM 1.2 (even the existing recovery hash feature). I guess the x86 guys will have to decide whether they think that's a problem. As far as I can tell all the Chrome OS boards using this are TPM 2.0, there are some Lenovo boards also setting HAS_RECOVERY_MRC_CACHE, but that was probably just copied over and I don't think we have non-Chrome OS users that really have use a full "recovery mode" like we do. The region is still protected by PRRs too anyway. So maybe it's fine?
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/secdata_... PS4, Line 169: assert(!CONFIG(TPM1)); This is already in the #if CONFIG(TPM2) block (see line 89), so this check is redundant. (BTW, if you're ever bored and looking for something to do, ;) I've been saying for years that we should split the TPM1/TPM2 parts of this file into separate files because that one #if somewhere up there is super hard to notice...)
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/secdata_... PS4, Line 173: HASH_NV_SIZE, nit: does this not fit on the previous line?