Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
mrc_cache: Add tpm_hash_index field to cache_region struct
Pull selection of tpm hash index logic into cache_region struct.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I1a744d6f40f062ca3aab6157b3747e6c1f6977f9 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/mrc_cache/mrc_cache.c 1 file changed, 18 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/46514/1
diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index 69f00e7..8124b24 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -46,6 +46,7 @@ uint32_t cbmem_id; int type; int elog_slot; + uint32_t tpm_hash_index; int flags; };
@@ -54,6 +55,11 @@ .cbmem_id = CBMEM_ID_MRCDATA, .type = MRC_TRAINING_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_RECOVERY, +#if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM) + .tpm_hash_index = MRC_REC_HASH_NV_INDEX, +#else + .tpm_hash_index = 0, +#endif #if CONFIG(HAS_RECOVERY_MRC_CACHE) .flags = RECOVERY_FLAG, #else @@ -66,6 +72,7 @@ .cbmem_id = CBMEM_ID_MRCDATA, .type = MRC_TRAINING_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_NORMAL, + .tpm_hash_index = MRC_RW_HASH_NV_INDEX, .flags = NORMAL_FLAG | RECOVERY_FLAG, };
@@ -74,6 +81,7 @@ .cbmem_id = CBMEM_ID_VAR_MRCDATA, .type = MRC_VARIABLE_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_VARIABLE, + .tpm_hash_index = 0, .flags = NORMAL_FLAG | RECOVERY_FLAG, };
@@ -176,12 +184,12 @@ return 0; }
-static int mrc_data_valid(const struct mrc_metadata *md, +static int mrc_data_valid(int type, const struct mrc_metadata *md, void *data, size_t data_size) { uint16_t checksum; - uint32_t hash_idx = vboot_recovery_mode_enabled() ? - MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX; + struct region region; + uint32_t hash_idx = lookup_region(®ion, type)->tpm_hash_index;
if (md->data_size != data_size) return -1; @@ -194,7 +202,8 @@ return -1; }
- if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(hash_idx, data, data_size)) + if (hash_idx && CONFIG(MRC_SAVE_HASH_IN_TPM) && + !mrc_cache_verify_hash(hash_idx, data, data_size)) return -1;
return 0; @@ -289,7 +298,7 @@ if (rdev_readat(&rdev, buffer, 0, data_size) != data_size) return -1;
- if (mrc_data_valid(&md, buffer, data_size) < 0) + if (mrc_data_valid(type, &md, buffer, data_size) < 0) return -1;
return data_size; @@ -316,7 +325,7 @@ return NULL; }
- if (mrc_data_valid(&md, data, region_device_size) < 0) + if (mrc_data_valid(type, &md, data, region_device_size) < 0) return NULL;
return data; @@ -396,8 +405,7 @@ const struct region_device *backing_rdev; struct region_device latest_rdev; const bool fail_bad_data = false; - uint32_t hash_idx = vboot_recovery_mode_enabled() ? - MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX; + uint32_t hash_idx;
cr = lookup_region(®ion, type);
@@ -457,7 +465,8 @@ } else { printk(BIOS_DEBUG, "MRC: updated '%s'.\n", cr->name); log_event_cache_update(cr->elog_slot, UPDATE_SUCCESS); - if (CONFIG(MRC_SAVE_HASH_IN_TPM)) + hash_idx = cr->tpm_hash_index; + if (hash_idx && CONFIG(MRC_SAVE_HASH_IN_TPM)) mrc_cache_update_hash(hash_idx, new_data, new_data_size); } }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 58: #if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM) This should be unnecessary, you're already checking MRC_SAVE_HASH_IN_TPM in the code, and if HAS_RECOVERY_MRC_CACHE isn't true it will never try using this cache_region anyway.
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 192: uint32_t hash_idx = lookup_region(®ion, type)->tpm_hash_index; nit: All the other lookup_region() calls check the result for NULL so should probably do that here too, or at least have an assert().
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46514
to look at the new patch set (#2).
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
mrc_cache: Add tpm_hash_index field to cache_region struct
Pull selection of tpm hash index logic into cache_region struct.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I1a744d6f40f062ca3aab6157b3747e6c1f6977f9 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/mrc_cache/mrc_cache.c M src/security/vboot/mrc_cache_hash_tpm.c 2 files changed, 19 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/46514/2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 58: #if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM)
This should be unnecessary, you're already checking MRC_SAVE_HASH_IN_TPM in the code, and if HAS_REC […]
Done
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 192: uint32_t hash_idx = lookup_region(®ion, type)->tpm_hash_index;
nit: All the other lookup_region() calls check the result for NULL so should probably do that here t […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46514/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46514/2//COMMIT_MSG@9 PS2, Line 9: Pull selection of tpm hash index logic into cache_region struct. Can you please also add the detail that this commit is enabling the support for saving MRC hash in TPM for both recovery and non-recovery cases?
This is a difference in observed behavior on all platforms using TPM2 and MRC cache driver.
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 58: #if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM)
This should be unnecessary, you're already checking MRC_SAVE_HASH_IN_TPM in the code, and if HAS_REC […]
Yes, check for MRC_SAVE_HASH_IN_TPM is not required. But, I think you would still need HAS_RECOVERY_MRC_CACHE.
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... PS2, Line 187: struct region region; This is not required.
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... PS2, Line 188: lookup_region(®ion, type); This needs to be `lookup_region_type()`. `lookup_region` also finds the region on SPI flash using `fmap_locate_area()` which is not required. This function only cares about getting a pointer to cache_region entry, which is provided by `lookup_region_type()`.
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46514
to look at the new patch set (#3).
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
mrc_cache: Add tpm_hash_index field to cache_region struct
Pull selection of tpm hash index logic into cache_region struct. This CL also enables the storing of the MRC hash into the TPM NVRAM space for both recovery and non-recovery cases. This will affect all platforms with TPM2 enabled and use the MRC_CACHE driver.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I1a744d6f40f062ca3aab6157b3747e6c1f6977f9 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/mrc_cache/mrc_cache.c M src/security/vboot/mrc_cache_hash_tpm.c 2 files changed, 22 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/46514/3
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46514/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46514/2//COMMIT_MSG@9 PS2, Line 9: Pull selection of tpm hash index logic into cache_region struct.
Can you please also add the detail that this commit is enabling the support for saving MRC hash in T […]
Done
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 58: #if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM)
Yes, check for MRC_SAVE_HASH_IN_TPM is not required. […]
Added HAS_RECOVERY_MRC_CACHE back in.
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... PS2, Line 187: struct region region;
This is not required.
Done
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... PS2, Line 188: lookup_region(®ion, type);
This needs to be `lookup_region_type()`. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 58: #if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM)
Added HAS_RECOVERY_MRC_CACHE back in.
Woops. Sorry, that was actually a stale comment before I looked into the call flows. Agree with Julius that the condition for HAS_RECOVERY_MRC_CACHE is not really required for tpm_hash_index.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
LGTM after the condition for HAS_RECOVERY_MRC_CACHE is dropped for tpm_hash_index.
https://review.coreboot.org/c/coreboot/+/46514/3/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/3/src/drivers/mrc_cache/mrc_c... PS3, Line 58: HAS_RECOVERY_MRC_CACHE Sorry about the confusion. I had a stale comment on earlier patchset which I missed removing before posting the latest comments on patchset 2. As Julius mentioned, this is not really required.
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46514
to look at the new patch set (#4).
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
mrc_cache: Add tpm_hash_index field to cache_region struct
Pull selection of tpm hash index logic into cache_region struct. This CL also enables the storing of the MRC hash into the TPM NVRAM space for both recovery and non-recovery cases. This will affect all platforms with TPM2 enabled and use the MRC_CACHE driver.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I1a744d6f40f062ca3aab6157b3747e6c1f6977f9 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/mrc_cache/mrc_cache.c M src/security/vboot/mrc_cache_hash_tpm.c 2 files changed, 18 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/46514/4
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46514/3/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/3/src/drivers/mrc_cache/mrc_c... PS3, Line 58: HAS_RECOVERY_MRC_CACHE
Sorry about the confusion. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 4: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
mrc_cache: Add tpm_hash_index field to cache_region struct
Pull selection of tpm hash index logic into cache_region struct. This CL also enables the storing of the MRC hash into the TPM NVRAM space for both recovery and non-recovery cases. This will affect all platforms with TPM2 enabled and use the MRC_CACHE driver.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I1a744d6f40f062ca3aab6157b3747e6c1f6977f9 Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46514 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/mrc_cache/mrc_cache.c M src/security/vboot/mrc_cache_hash_tpm.c 2 files changed, 18 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index 8d4df8f..1bbb426 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -46,6 +46,7 @@ uint32_t cbmem_id; int type; int elog_slot; + uint32_t tpm_hash_index; int flags; };
@@ -54,6 +55,7 @@ .cbmem_id = CBMEM_ID_MRCDATA, .type = MRC_TRAINING_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_RECOVERY, + .tpm_hash_index = MRC_REC_HASH_NV_INDEX, #if CONFIG(HAS_RECOVERY_MRC_CACHE) .flags = RECOVERY_FLAG, #else @@ -66,6 +68,7 @@ .cbmem_id = CBMEM_ID_MRCDATA, .type = MRC_TRAINING_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_NORMAL, + .tpm_hash_index = MRC_RW_HASH_NV_INDEX, .flags = NORMAL_FLAG | RECOVERY_FLAG, };
@@ -74,6 +77,7 @@ .cbmem_id = CBMEM_ID_VAR_MRCDATA, .type = MRC_VARIABLE_DATA, .elog_slot = ELOG_MEM_CACHE_UPDATE_SLOT_VARIABLE, + .tpm_hash_index = 0, .flags = NORMAL_FLAG | RECOVERY_FLAG, };
@@ -176,11 +180,15 @@ return 0; }
-static int mrc_data_valid(const struct mrc_metadata *md, +static int mrc_data_valid(int type, const struct mrc_metadata *md, void *data, size_t data_size) { uint16_t checksum; - uint32_t hash_idx = MRC_REC_HASH_NV_INDEX; + const struct cache_region *cr = lookup_region_type(type); + uint32_t hash_idx; + + if (cr == NULL) + return -1;
if (md->data_size != data_size) return -1; @@ -193,7 +201,9 @@ return -1; }
- if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(hash_idx, data, data_size)) + hash_idx = cr->tpm_hash_index; + if (hash_idx && CONFIG(MRC_SAVE_HASH_IN_TPM) && + !mrc_cache_verify_hash(hash_idx, data, data_size)) return -1;
return 0; @@ -288,7 +298,7 @@ if (rdev_readat(&rdev, buffer, 0, data_size) != data_size) return -1;
- if (mrc_data_valid(&md, buffer, data_size) < 0) + if (mrc_data_valid(type, &md, buffer, data_size) < 0) return -1;
return data_size; @@ -315,7 +325,7 @@ return NULL; }
- if (mrc_data_valid(&md, data, region_device_size) < 0) + if (mrc_data_valid(type, &md, data, region_device_size) < 0) return NULL;
return data; @@ -395,7 +405,7 @@ const struct region_device *backing_rdev; struct region_device latest_rdev; const bool fail_bad_data = false; - uint32_t hash_idx = MRC_REC_HASH_NV_INDEX; + uint32_t hash_idx;
cr = lookup_region(®ion, type);
@@ -455,7 +465,8 @@ } else { printk(BIOS_DEBUG, "MRC: updated '%s'.\n", cr->name); log_event_cache_update(cr->elog_slot, UPDATE_SUCCESS); - if (CONFIG(MRC_SAVE_HASH_IN_TPM)) + hash_idx = cr->tpm_hash_index; + if (hash_idx && CONFIG(MRC_SAVE_HASH_IN_TPM)) mrc_cache_update_hash(hash_idx, new_data, new_data_size); } } diff --git a/src/security/vboot/mrc_cache_hash_tpm.c b/src/security/vboot/mrc_cache_hash_tpm.c index fede488..77c23f6 100644 --- a/src/security/vboot/mrc_cache_hash_tpm.c +++ b/src/security/vboot/mrc_cache_hash_tpm.c @@ -24,10 +24,6 @@ }; const uint8_t *hash_ptr = data_hash;
- /* We do not store normal mode data hash in TPM. */ - if (!vboot_recovery_mode_enabled()) - return; - /* Initialize TPM driver. */ if (tlcl_lib_init() != VB2_SUCCESS) { printk(BIOS_ERR, "MRC: TPM driver initialization failed.\n"); @@ -64,10 +60,6 @@ uint8_t data_hash[VB2_SHA256_DIGEST_SIZE]; uint8_t tpm_hash[VB2_SHA256_DIGEST_SIZE];
- /* We do not store normal mode data hash in TPM. */ - if (!vboot_recovery_mode_enabled()) - return 1; - /* Calculate hash of data read from MRC_CACHE. */ if (vb2_digest_buffer(data, size, VB2_HASH_SHA256, data_hash, sizeof(data_hash))) {
-
Furquan Shaikh (Code Review) -
Julius Werner (Code Review)
-
Shelley Chen (Code Review)