Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46434 )
Change subject: mrc_cache: Add TPM Hash verification ......................................................................
Patch Set 5:
(3 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/46434/5/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46434/5/src/drivers/mrc_cache/mrc_c... PS5, Line 181: checksum = compute_ip_checksum(data, data_size); Do we want to replace this w/ the mrc_cache_verify_hash when MRC_SAVE_MRC_HASH is enabled? Seems like we would want to instead of calculating things over the data multiple times.
https://review.coreboot.org/c/coreboot/+/46434/5/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46434/5/src/security/vboot/mrc_cach... PS5, Line 13: void mrc_cache_update_hash(const uint8_t *data, size_t size) I feel like we should pass in hash objects that are calculated outside of these functions. It feels like we're baking a lot policy in this file now. I feel like it should be more a library where the control flow is implemented elsewhere.
https://review.coreboot.org/c/coreboot/+/46434/5/src/security/vboot/mrc_cach... PS5, Line 58: /* Lock TPM space */ Why is this the correct place to lock the space? Shouldn't that be a separate policy for when we should do that instead of embedding this in this API call?