Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46432 )
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 4 files changed, 43 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/1
diff --git a/src/security/vboot/antirollback.h b/src/security/vboot/antirollback.h index 595205d..896252e 100644 --- a/src/security/vboot/antirollback.h +++ b/src/security/vboot/antirollback.h @@ -23,7 +23,9 @@ #define BACKUP_NV_INDEX 0x1009 #define FWMP_NV_INDEX 0x100a #define REC_HASH_NV_INDEX 0x100b -#define REC_HASH_NV_SIZE VB2_SHA256_DIGEST_SIZE +/* 0x100c is used for OOBE autoconfig public key hashes */ +#define NORM_HASH_NV_INDEX 0x100d +#define HASH_NV_SIZE VB2_SHA256_DIGEST_SIZE
/* Structure definitions for TPM spaces */
@@ -56,10 +58,10 @@ uint32_t antirollback_lock_space_firmware(void);
/* Read recovery hash data from TPM. */ -uint32_t antirollback_read_space_rec_hash(uint8_t *data, uint32_t size); +uint32_t antirollback_read_space_hash(uint32_t index, uint8_t *data, uint32_t size); /* Write new hash data to recovery space in TPM. */ -uint32_t antirollback_write_space_rec_hash(const uint8_t *data, uint32_t size); +uint32_t antirollback_write_space_hash(uint32_t index, const uint8_t *data, uint32_t size); /* Lock down recovery hash space in TPM. */ -uint32_t antirollback_lock_space_rec_hash(void); +uint32_t antirollback_lock_space_hash(uint32_t index);
#endif /* ANTIROLLBACK_H_ */ diff --git a/src/security/vboot/mrc_cache_hash_tpm.c b/src/security/vboot/mrc_cache_hash_tpm.c index bc500a2..c5be1b1 100644 --- a/src/security/vboot/mrc_cache_hash_tpm.c +++ b/src/security/vboot/mrc_cache_hash_tpm.c @@ -23,10 +23,8 @@ 0xba, 0xad, 0xba, 0xad, /* BAADBAAD */ }; const uint8_t *hash_ptr = data_hash; - - /* We do not store normal mode data hash in TPM. */ - if (!vboot_recovery_mode_enabled()) - return; + uint32_t hash_idx = vboot_recovery_mode_enabled() ? + REC_HASH_NV_INDEX : NORM_HASH_NV_INDEX;
/* Initialize TPM driver. */ if (tlcl_lib_init() != VB2_SUCCESS) { @@ -50,7 +48,7 @@ }
/* Write hash of data to TPM space. */ - if (antirollback_write_space_rec_hash(hash_ptr, VB2_SHA256_DIGEST_SIZE) + if (antirollback_write_space_hash(hash_idx, hash_ptr, VB2_SHA256_DIGEST_SIZE) != TPM_SUCCESS) { printk(BIOS_ERR, "MRC: Could not save hash to TPM.\n"); return; @@ -63,10 +61,8 @@ { 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; + uint32_t hash_idx = vboot_recovery_mode_enabled() ? + REC_HASH_NV_INDEX : NORM_HASH_NV_INDEX;
/* Calculate hash of data read from RECOVERY_MRC_CACHE. */ if (vb2_digest_buffer(data, size, VB2_HASH_SHA256, data_hash, @@ -82,7 +78,7 @@ }
/* Read hash of MRC data saved in TPM. */ - if (antirollback_read_space_rec_hash(tpm_hash, sizeof(tpm_hash)) + if (antirollback_read_space_hash(hash_idx, tpm_hash, sizeof(tpm_hash)) != TPM_SUCCESS) { printk(BIOS_ERR, "MRC: Could not read hash from TPM.\n"); return 0; @@ -94,6 +90,6 @@ }
printk(BIOS_INFO, "MRC: Hash comparison successful. " - "Using data from RECOVERY_MRC_CACHE\n"); + "Using data from MRC_CACHE\n"); return 1; } diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 691d2c0..c907109 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -71,10 +71,10 @@ return TPM_SUCCESS; }
-static uint32_t read_space_rec_hash(uint8_t *data) +static uint32_t read_space_hash(uint32_t index, uint8_t *data) { - RETURN_ON_FAILURE(tlcl_read(REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE)); + RETURN_ON_FAILURE(tlcl_read(index, data, + HASH_NV_SIZE)); return TPM_SUCCESS; }
@@ -83,7 +83,7 @@ * it. Since there is no data available to calculate hash at the point where TPM * space is defined, initialize it to all 0s. */ -static const uint8_t rec_hash_data[REC_HASH_NV_SIZE] = { }; +static const uint8_t rec_hash_data[HASH_NV_SIZE] = { };
#if CONFIG(TPM2) /* @@ -162,10 +162,10 @@ VB2_SECDATA_KERNEL_SIZE, rw_space_attributes, NULL, 0); }
-static uint32_t set_rec_hash_space(const uint8_t *data) +static uint32_t set_hash_space(uint32_t index, const uint8_t *data) { - return set_space("MRC Hash", REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE, + return set_space("MRC Hash", index, data, + HASH_NV_SIZE, ro_space_attributes, pcr0_unchanged_policy, sizeof(pcr0_unchanged_policy)); } @@ -185,7 +185,7 @@ RETURN_ON_FAILURE(set_kernel_space(ctx->secdata_kernel));
if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) - RETURN_ON_FAILURE(set_rec_hash_space(rec_hash_data)); + RETURN_ON_FAILURE(set_hash_space(REC_HASH_NV_INDEX, rec_hash_data));
RETURN_ON_FAILURE(set_firmware_space(ctx->secdata_firmware));
@@ -197,9 +197,9 @@ return tlcl_lock_nv_write(FIRMWARE_NV_INDEX); }
-uint32_t antirollback_lock_space_rec_hash(void) +uint32_t antirollback_lock_space_hash(uint32_t index) { - return tlcl_lock_nv_write(REC_HASH_NV_INDEX); + return tlcl_lock_nv_write(index); }
#else @@ -239,14 +239,14 @@ } }
-static uint32_t set_rec_hash_space(const uint8_t *data) +static uint32_t set_hash_space(uint32_t index, const uint8_t *data) { - RETURN_ON_FAILURE(safe_define_space(REC_HASH_NV_INDEX, + RETURN_ON_FAILURE(safe_define_space(index, TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE, - REC_HASH_NV_SIZE)); - RETURN_ON_FAILURE(safe_write(REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE)); + HASH_NV_SIZE)); + RETURN_ON_FAILURE(safe_write(index, data, + HASH_NV_SIZE));
return TPM_SUCCESS; } @@ -307,7 +307,7 @@
/* Define and set rec hash space, if available. */ if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) - RETURN_ON_FAILURE(set_rec_hash_space(rec_hash_data)); + RETURN_ON_FAILURE(set_hash_space(REC_HASH_NV_INDEX, rec_hash_data));
return TPM_SUCCESS; } @@ -317,7 +317,7 @@ return tlcl_set_global_lock(); }
-uint32_t antirollback_lock_space_rec_hash(void) +uint32_t antirollback_lock_space_hash(void) { /* * Nothing needs to be done here, since global lock is already set while @@ -417,43 +417,43 @@ return safe_write(KERNEL_NV_INDEX, ctx->secdata_kernel, size); }
-uint32_t antirollback_read_space_rec_hash(uint8_t *data, uint32_t size) +uint32_t antirollback_read_space_hash(uint32_t index, uint8_t *data, uint32_t size) { - if (size != REC_HASH_NV_SIZE) { + if (size != HASH_NV_SIZE) { VBDEBUG("TPM: Incorrect buffer size for rec hash. " - "(Expected=0x%x Actual=0x%x).\n", REC_HASH_NV_SIZE, + "(Expected=0x%x Actual=0x%x).\n", HASH_NV_SIZE, size); return TPM_E_READ_FAILURE; } - return read_space_rec_hash(data); + return read_space_hash(index, data); }
-uint32_t antirollback_write_space_rec_hash(const uint8_t *data, uint32_t size) +uint32_t antirollback_write_space_hash(uint32_t index, const uint8_t *data, uint32_t size) { - uint8_t spc_data[REC_HASH_NV_SIZE]; + uint8_t spc_data[HASH_NV_SIZE]; uint32_t rv;
- if (size != REC_HASH_NV_SIZE) { + if (size != HASH_NV_SIZE) { VBDEBUG("TPM: Incorrect buffer size for rec hash. " - "(Expected=0x%x Actual=0x%x).\n", REC_HASH_NV_SIZE, + "(Expected=0x%x Actual=0x%x).\n", HASH_NV_SIZE, size); return TPM_E_WRITE_FAILURE; }
- rv = read_space_rec_hash(spc_data); + rv = read_space_hash(index, spc_data); if (rv == TPM_E_BADINDEX) { /* * If space is not defined already for recovery hash, define * new space. */ - VBDEBUG("TPM: Initializing recovery hash space.\n"); - return set_rec_hash_space(data); + VBDEBUG("TPM: Initializing hash space.\n"); + return set_hash_space(index, data); }
if (rv != TPM_SUCCESS) return rv;
- return safe_write(REC_HASH_NV_INDEX, data, size); + return safe_write(index, data, size); }
vb2_error_t vb2ex_tpm_clear_owner(struct vb2_context *ctx) diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 0f18f9a..873e796 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -404,7 +404,7 @@
/* Lock rec hash space if available. */ if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) { - rv = antirollback_lock_space_rec_hash(); + rv = antirollback_lock_space_hash(REC_HASH_NV_INDEX); if (rv) { printk(BIOS_INFO, "Failed to lock rec hash space(%x)\n", rv);
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 1:
(9 comments)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/antiroll... PS1, Line 27: NORM_HASH_NV_INDEX I'd call this MRC_HASH_NV_INDEX (or something like MRC_RW_HASH_NV_INDEX if you want to be specific) because otherwise there's no indication that this has anything to do with MRC cache. (The other one should've probably been called MRC_REC_HASH_NV_INDEX or something like that...)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 28: if (!vboot_recovery_mode_enabled()) Removing this will immediately enable this on the FSP 2.0 platforms that already use the recovery cache hash. Generally I don't see a problem with using this everywhere, but those platforms also already use platform-specific flash protection for MRC cache so maybe they'd consider it redundant and unnecessary. +Furquan should decide whether this is okay. (Also because of the TPM 1.2 problem I mentioned that exists for the RW hash but not for the recovery hash, maybe we should still keep both hashes separate in Kconfig so that boards with TPM 1.2 have the option to enable only one but not the other.)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 36: if (vb2_digest_buffer(data, size, VB2_HASH_SHA256, data_hash, nit: In practice this function can't fail unless the arguments are wrong, so if you ask me this could also just be an assert() and we wouldn't need that whole dead_hash stuff. But somewhat off-topic.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 57: printk(BIOS_INFO, "MRC: TPM MRC hash updated successfully.\n"); nit: might be nice to also print the index here now
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 93: "Using data from MRC_CACHE\n"); nit: here too (and I'm not sure what "Using data from MRC_CACHE" is really supposed to tell us here, isn't that implied when you talk about "MRC: Hash comparison"?)
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 169: ro_space_attributes This is the tricky part: for the RW hash, this needs to be rw_space_attributes and no policy (similar to what set_kernel_space() above does).
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.2 (at least not without making things way more complicated). So we should just prevent that with Kconfig dependencies and assert() here that it can't happen or something.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 423: rec nit: remove
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 437: rec here too
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 1:
(1 comment)
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 420: uint32_t antirollback_read_space_hash(uint32_t index, uint8_t *data, uint32_t size) BTW there are stub versions of these functions in secdata_mock.c that you need to update as well.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#2).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 51 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/2
Shelley Chen 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 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/antiroll... PS1, Line 27: NORM_HASH_NV_INDEX
I'd call this MRC_HASH_NV_INDEX (or something like MRC_RW_HASH_NV_INDEX if you want to be specific) […]
Done
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 57: printk(BIOS_INFO, "MRC: TPM MRC hash updated successfully.\n");
nit: might be nice to also print the index here now
Done.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 93: "Using data from MRC_CACHE\n");
nit: here too (and I'm not sure what "Using data from MRC_CACHE" is really supposed to tell us here, […]
Done.
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 420: uint32_t antirollback_read_space_hash(uint32_t index, uint8_t *data, uint32_t size)
BTW there are stub versions of these functions in secdata_mock.c that you need to update as well.
Done
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 423: rec
nit: remove
Done
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 437: rec
here too
Done
Aaron Durbin 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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/antiroll... PS2, Line 27: MRC_ Why did we add MRC_ to this? I think we should be consistent with REC_HASH_NV_INDEX and MRC_RW_HASH_NV_INDEX.
REC_MEM_HASH_NV_INDEX NORM_MEM_HASH_NV_INDEX or RW_MEM_HASH_NV_INDEX
Also, we should probably add a comment explaining what is stored at these indicies.
It looks like Julius commented something similar. I suggest we align the names, but that can be in a follow up.
Aaron Durbin 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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/mrc_cach... PS2, Line 67: /* Calculate hash of data read from RECOVERY_MRC_CACHE. */ This comment is stale.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#3).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 59 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/3
Shelley Chen 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/2/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/antiroll... PS2, Line 27: MRC_
Why did we add MRC_ to this? I think we should be consistent with REC_HASH_NV_INDEX and MRC_RW_HASH_ […]
Renamed REC_HASH_NV_INDEX to MRC_REC_HASH_NV_INDEX per Julius' suggestion.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 28: if (!vboot_recovery_mode_enabled())
Removing this will immediately enable this on the FSP 2. […]
What do you think Furquan?
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/2/src/security/vboot/mrc_cach... PS2, Line 67: /* Calculate hash of data read from RECOVERY_MRC_CACHE. */
This comment is stale.
Done. Removed recovery reference.
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 169: ro_space_attributes
This is the tricky part: for the RW hash, this needs to be rw_space_attributes and no policy (simila […]
Done
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.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#4).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami and lazor
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 67 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/4
Shelley Chen 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:
(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.
Done
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)
(Still open, btw)
Done in https://review.coreboot.org/c/coreboot/+/46434
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 crea […]
Adding the if condition removed the need for this variable.
https://review.coreboot.org/c/coreboot/+/46432/3/src/security/vboot/secdata_... PS3, Line 168: "MRC Hash"
Might also be nice to change this... […]
Done
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?
Furquan Shaikh 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:
(8 comments)
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 28: normal nit: non-recovery
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 62: recovery hash comment needs update here and for the functions below.
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 63: antirollback_read_space_hash Probably add _mrc_ in the name to make it clear that these functions deal with hash of MRC data.
Also, can you please add a comment indicating that index can be either MRC_REC_HASH_NV_INDEX or MRC_RW_HASH_NV_INDEX depending upon the mode for which the hash of the MRC data is calculated.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 28: if (!vboot_recovery_mode_enabled())
What do you think Furquan?
I think it's okay to have the RW MRC hash enabled for FSP2.0 platforms that were using recovery hash space already. But rather than silently changing it here, we should do this in proper sequence of CLs and documenting the impact in commit message.
I am thinking the CL train should look something like:
CL-1: Header clean up - Move drivers/intel/fsp2_0/include/fsp/memory_init.h into security/vboot/mrc_cache_hash_tpm.h - Include mrc_cache_hash_tpm.h instead of memory_init.h.
CL-2: Kconfig variable clean up - Add new Kconfig in drivers/mrc_cache/Kconfig: `MRC_SAVE_HASH_IN_TPM` which depends on VBOOT && TPM2 && !TPM1. I don't think this should be set to 'y' by default. It should be left to the individual platform to decide this. - In drivers/intel/fsp2_0/Kconfig, drop `FSP2_0_USES_TPM_MRC_HASH` and instead auto-select `MRC_SAVE_HASH_IN_TPM` for `PLATFORM_USES_FSP2_0` if `HAS_RECOVERY_MRC_CACHE` is selected. - Replace all uses of `FSP2_0_USES_TPM_MRC_HASH` with `MRC_SAVE_HASH_IN_TPM`. `VBOOT_HAS_REC_HASH_SPACE` should be set to default 'y' when `MRC_SAVE_HASH_IN_TPM` and `HAS_RECOVERY_MRC_CACHE` is selected.
CL-3: - Move the calls to `mrc_cache_{update,verify}_hash` from drivers/intel/fsp2_0/memory_init.c to drivers/mrc_cache/mrc_cache.c (This change would still be saving the hash only in recovery mode).
CL-4: - Add support for index parameter in all required functions (antirollback_*, mrc_cache_update_hash, mrc_cache_verify_hash). - In drivers/mrc_cache/mrc_cache.c, call `mrc_cache_{update,verify}_hash()` functions with `MRC_REC_HASH_NV_INDEX` if boot is recovery mode. Drop the check for recovery mode from `mrc_cache_{update,verify}_hash()` functions. (This change would still be saving the hash only in recovery mode, but the decision is moved to mrc_cache.c from mrc_cache_hash_tpm.c).
CL-5: - Add new index for `MRC_RW_HASH_NV_INDEX` and update the `antirollback_*()` functions to handle this index.
CL-6: - Add a new member to `struct cache_region` in mrc_cache.c - `uint32_t tpm_hash_index;` which indicates what index to pass to `mrc_cache_{update,verify}_hash()` functions. - In `normal_training` structure, set `tpm_hash_index` to `MRC_RW_HASH_NV_INDEX` if `MRC_SAVE_HASH_IN_TPM` is selected. - In `recovery_training` structure, set `tpm_hash_index` to `MRC_REC_HASH_NV_INDEX` if `MRC_SAVE_HASH_IN_TPM` and `HAS_RECOVERY_MRC_CACHE` are selected. - In `variable_data` structure, set `tpm_hash_index` to 0. - Call `mrc_cache_{update,verify}_hash()` functions only if tpm_hash_index is non-zero. - Change the selection of `MRC_SAVE_HASH_IN_TPM` by `PLATFORM_USES_FSP2_0` if `CHROMEOS` instead of `HAS_RECOVERY_MRC_CACHE`. (This CL actually uses both recovery and non-recovery hash spaces when a platform is using FSP2.0 and CHROMEOS. This change might also need a little refactoring to avoid calling `lookup_region_type()` multiple times.)
CL-7: - We can optimize by skipping the checksum calculation when tpm_hash_index is non-zero and we are making call to `mrc_cache_verify_hash()`.
CL-8: - Drop support in TPM1.2 for reading/writing/locking recovery mrc hash space in TPM.
I know it's breaking down into a large number of smaller CLs, but I think that will help in also identifying any regression if we miss out on some platform quirks. I can help push some CLs as well if you need some help.
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... PS4, Line 26: uint32_t hash_idx = vboot_recovery_mode_enabled() ? : MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX; Instead of identifying what hash_idx to use here, I think it would be better to let the caller implement that policy and pass in the index.
And we can use `struct cache_region` to encode the index that needs to be used for a particular type of cache. It allows the caller flexibility to decide when/if to use the hash functionality for a particular mode and cache type. You can also get rid of the dependency on !MRC_SETTINGS_VARIABLE_DATA in the following CL. I think that is just artificially restricting platforms that have variable data from being able to use the hash in TPM functionality.
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... PS4, Line 41: recovery Comment will need update.
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)
Hmmm... okay so we're just completely forbidding it for TPM 1. […]
I think that should be okay. All the platforms that are using RECOVERY_MRC_CACHE and the recovery mrc hash space in TPM are TPM2.0 platforms. The lenovo platforms don't use FSP2.0, so the hash in TPM Kconfig never gets enabled for these.
I think we can drop the support of hash in TPM for TPM1.2 completely. (We should clean up the functions in secdata_tpm.c to print out error and not really do anything with the TPM space).
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 199: Is the initialization of MRC_RW_HASH_NV_INDEX intentionally skipped here?
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#5).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 64 insertions(+), 58 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/5
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#6).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 64 insertions(+), 58 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/6
Shelley Chen 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 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 28: if (!vboot_recovery_mode_enabled())
I think it's okay to have the RW MRC hash enabled for FSP2. […]
For now I've split the current CL into CL1-5. I still have to do CL6-8.
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. […]
Removed.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#8).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 67 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/8
Shelley Chen 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 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 62: recovery hash
comment needs update here and for the functions below.
Done
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 63: antirollback_read_space_hash
Probably add _mrc_ in the name to make it clear that these functions deal with hash of MRC data. […]
Done
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... PS4, Line 41: recovery
Comment will need update.
Done
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 173: HASH_NV_SIZE,
nit: does this not fit on the previous line?
Moved up.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#9).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 67 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/9
Shelley Chen 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 9:
(1 comment)
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 199:
Is the initialization of MRC_RW_HASH_NV_INDEX intentionally skipped here?
Done in https://review.coreboot.org/c/coreboot/+/46511
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#10).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 67 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/10
Furquan Shaikh 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 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/antirol... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/antirol... PS10, Line 61: MRC_RW_HASH_NV_INDEX nit: This is being introduced later.
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... PS10, Line 12: mrc_cache_update_hash I think this and the function below should be updated to take in index as an argument.
Furquan Shaikh 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 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... PS10, Line 27: /* We do not store normal mode data hash in TPM. */ : if (!vboot_recovery_mode_enabled()) : return; It is not correct to drop this. It should be dropped when complete support for both rec and non-rec indexes is added.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#11).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 78 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/11
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 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... PS10, Line 171: } else { This part should probably also go to the next patch.
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... PS10, Line 246: static uint32_t set_mrc_hash_space(uint32_t index, const uint8_t *data) Since you now already restricted this to TPM2 in the last patch, you could also just take out all of this stuff from the TPM1 parts and just move the toplevel API entry points into the TPM2 block.
Shelley Chen 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 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... PS10, Line 171: } else {
This part should probably also go to the next patch.
But we still need to correctly accommodate the non-recovery case here right (even though the index doesn't exist yet)?
Furquan Shaikh 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 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... PS10, Line 171: } else {
But we still need to correctly accommodate the non-recovery case here right (even though the index d […]
That would have to be done where you actually introduce the index. This should really get called only for recovery case at this point in the patch train.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#14).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 71 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/14
Shelley Chen 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 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... PS10, Line 27: /* We do not store normal mode data hash in TPM. */ : if (!vboot_recovery_mode_enabled()) : return;
It is not correct to drop this. […]
Moved to https://review.coreboot.org/c/coreboot/+/46511
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... PS10, Line 171: } else {
That would have to be done where you actually introduce the index. […]
Done
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#15).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 5 files changed, 71 insertions(+), 55 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/15
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46432
to look at the new patch set (#16).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/mrc_cache/mrc_cache.c M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/mrc_cache_hash_tpm.h M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 7 files changed, 78 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/16
Shelley Chen 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 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/mrc_cac... PS10, Line 12: mrc_cache_update_hash
I think this and the function below should be updated to take in index as an argument.
Done
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 16: Code-Review+1
LGTM, I'll leave the +2 to Furquan.
Furquan Shaikh 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 16: Code-Review+1
(4 comments)
`
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 62: data from which hash is to be computed That is not correct. This is a pointer to a data buffer where the hash from TPM is read into.
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 63: size of data from which has is to be computed size of the data buffer where the hash from TPM is read into.
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 69: data from which hash is to be computed Same comment as above.
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 70: size of data from which has is to be computed Same here.
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/+/46432
to look at the new patch set (#17).
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/mrc_cache/mrc_cache.c M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/mrc_cache_hash_tpm.h M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 7 files changed, 78 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/46432/17
Shelley Chen 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 17:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 62: data from which hash is to be computed
That is not correct. This is a pointer to a data buffer where the hash from TPM is read into.
Done
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 63: size of data from which has is to be computed
size of the data buffer where the hash from TPM is read into.
Done
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 69: data from which hash is to be computed
Same comment as above.
Done
https://review.coreboot.org/c/coreboot/+/46432/16/src/security/vboot/antirol... PS16, Line 70: size of data from which has is to be computed
Same here.
Done
Furquan Shaikh 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 17: Code-Review+2
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 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 28: if (!vboot_recovery_mode_enabled())
For now I've split the current CL into CL1-5. I still have to do CL6-8.
Done
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... PS4, Line 26: uint32_t hash_idx = vboot_recovery_mode_enabled() ? : MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX;
Instead of identifying what hash_idx to use here, I think it would be better to let the caller imple […]
Done
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/10/src/security/vboot/secdata... PS10, Line 246: static uint32_t set_mrc_hash_space(uint32_t index, const uint8_t *data)
Since you now already restricted this to TPM2 in the last patch, you could also just take out all of […]
Will be done in later CL.
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46432 )
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
security/vboot: Make mrc_cache hash functions generic
We need to extend the functionality of the mrc_cache hash functions to work for both recovery and normal mrc_cache data. Updating the API of these functions to pass in an index to identify the hash indices for recovery and normal mode.
BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami
Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46432 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/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c M src/security/vboot/mrc_cache_hash_tpm.h M src/security/vboot/secdata_mock.c M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 7 files changed, 78 insertions(+), 61 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 95abc4f..8d4df8f 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -10,6 +10,7 @@ #include <fmap.h> #include <ip_checksum.h> #include <region_file.h> +#include <security/vboot/antirollback.h> #include <security/vboot/mrc_cache_hash_tpm.h> #include <security/vboot/vboot_common.h> #include <spi_flash.h> @@ -179,6 +180,7 @@ void *data, size_t data_size) { uint16_t checksum; + uint32_t hash_idx = MRC_REC_HASH_NV_INDEX;
if (md->data_size != data_size) return -1; @@ -191,7 +193,7 @@ return -1; }
- if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(data, data_size)) + if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(hash_idx, data, data_size)) return -1;
return 0; @@ -393,6 +395,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;
cr = lookup_region(®ion, type);
@@ -453,7 +456,7 @@ 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)) - mrc_cache_update_hash(new_data, new_data_size); + mrc_cache_update_hash(hash_idx, new_data, new_data_size); } }
diff --git a/src/security/vboot/antirollback.h b/src/security/vboot/antirollback.h index 595205d..8b183da 100644 --- a/src/security/vboot/antirollback.h +++ b/src/security/vboot/antirollback.h @@ -22,8 +22,9 @@ * want to use 0x1009 for something else. */ #define BACKUP_NV_INDEX 0x1009 #define FWMP_NV_INDEX 0x100a -#define REC_HASH_NV_INDEX 0x100b -#define REC_HASH_NV_SIZE VB2_SHA256_DIGEST_SIZE +/* 0x100b: Hash of MRC_CACHE training data for recovery boot */ +#define MRC_REC_HASH_NV_INDEX 0x100b +#define HASH_NV_SIZE VB2_SHA256_DIGEST_SIZE
/* Structure definitions for TPM spaces */
@@ -55,11 +56,25 @@ */ uint32_t antirollback_lock_space_firmware(void);
-/* Read recovery hash data from TPM. */ -uint32_t antirollback_read_space_rec_hash(uint8_t *data, uint32_t size); -/* Write new hash data to recovery space in TPM. */ -uint32_t antirollback_write_space_rec_hash(const uint8_t *data, uint32_t size); -/* Lock down recovery hash space in TPM. */ -uint32_t antirollback_lock_space_rec_hash(void); +/* + * Read recovery hash data from TPM. + * @param index index into TPM NVRAM where hash is stored + * @param data pointer to buffer where hash from TPM read into + * @param size size of buffer + */ +uint32_t antirollback_read_space_mrc_hash(uint32_t index, uint8_t *data, uint32_t size); +/* + * Write new hash data to recovery space in TPM.\ + * @param index index into TPM NVRAM where hash is stored + * @param data pointer to buffer of hash value to be written + * @param size size of buffer +*/ +uint32_t antirollback_write_space_mrc_hash(uint32_t index, const uint8_t *data, + uint32_t size); +/* + * Lock down recovery hash space in TPM. + * @param index index into TPM NVRAM where hash is stored +*/ +uint32_t antirollback_lock_space_mrc_hash(uint32_t index);
#endif /* ANTIROLLBACK_H_ */ diff --git a/src/security/vboot/mrc_cache_hash_tpm.c b/src/security/vboot/mrc_cache_hash_tpm.c index 24e7aaf..fede488 100644 --- a/src/security/vboot/mrc_cache_hash_tpm.c +++ b/src/security/vboot/mrc_cache_hash_tpm.c @@ -9,7 +9,7 @@ #include <console/console.h> #include <string.h>
-void mrc_cache_update_hash(const uint8_t *data, size_t size) +void mrc_cache_update_hash(uint32_t index, const uint8_t *data, size_t size) { uint8_t data_hash[VB2_SHA256_DIGEST_SIZE]; static const uint8_t dead_hash[VB2_SHA256_DIGEST_SIZE] = { @@ -40,26 +40,26 @@ printk(BIOS_ERR, "MRC: SHA-256 calculation failed for data. " "Not updating TPM hash space.\n"); /* - * Since data is being updated in recovery cache, the hash - * currently stored in TPM recovery hash space is no longer - * valid. If we are not able to calculate hash of the data being - * updated, reset all the bits in TPM recovery hash space to - * pre-defined hash pattern. + * Since data is being updated in mrc cache, the hash + * currently stored in TPM hash space is no longer + * valid. If we are not able to calculate hash of the + * data being updated, reset all the bits in TPM hash + * space to pre-defined hash pattern. */ hash_ptr = dead_hash; }
/* Write hash of data to TPM space. */ - if (antirollback_write_space_rec_hash(hash_ptr, VB2_SHA256_DIGEST_SIZE) + if (antirollback_write_space_mrc_hash(index, hash_ptr, VB2_SHA256_DIGEST_SIZE) != TPM_SUCCESS) { printk(BIOS_ERR, "MRC: Could not save hash to TPM.\n"); return; }
- printk(BIOS_INFO, "MRC: TPM MRC hash updated successfully.\n"); + printk(BIOS_INFO, "MRC: TPM MRC hash idx 0x%x updated successfully.\n", index); }
-int mrc_cache_verify_hash(const uint8_t *data, size_t size) +int mrc_cache_verify_hash(uint32_t index, const uint8_t *data, size_t size) { uint8_t data_hash[VB2_SHA256_DIGEST_SIZE]; uint8_t tpm_hash[VB2_SHA256_DIGEST_SIZE]; @@ -68,7 +68,7 @@ if (!vboot_recovery_mode_enabled()) return 1;
- /* Calculate hash of data read from RECOVERY_MRC_CACHE. */ + /* Calculate hash of data read from MRC_CACHE. */ if (vb2_digest_buffer(data, size, VB2_HASH_SHA256, data_hash, sizeof(data_hash))) { printk(BIOS_ERR, "MRC: SHA-256 calculation failed for data.\n"); @@ -82,7 +82,7 @@ }
/* Read hash of MRC data saved in TPM. */ - if (antirollback_read_space_rec_hash(tpm_hash, sizeof(tpm_hash)) + if (antirollback_read_space_mrc_hash(index, tpm_hash, sizeof(tpm_hash)) != TPM_SUCCESS) { printk(BIOS_ERR, "MRC: Could not read hash from TPM.\n"); return 0; @@ -93,7 +93,7 @@ return 0; }
- printk(BIOS_INFO, "MRC: Hash comparison successful. " - "Using data from RECOVERY_MRC_CACHE\n"); + printk(BIOS_INFO, "MRC: Hash idx 0x%x comparison successful.\n", index); + return 1; } diff --git a/src/security/vboot/mrc_cache_hash_tpm.h b/src/security/vboot/mrc_cache_hash_tpm.h index a1ecd8b..cf443f2 100644 --- a/src/security/vboot/mrc_cache_hash_tpm.h +++ b/src/security/vboot/mrc_cache_hash_tpm.h @@ -8,12 +8,12 @@ /* * Updates mrc cache hash if it differs. */ -void mrc_cache_update_hash(const uint8_t *data, size_t size); +void mrc_cache_update_hash(uint32_t index, const uint8_t *data, size_t size);
/* * Verifies mrc cache hash which is stored somewhere. * return 1 verification was successful and 0 for error. */ -int mrc_cache_verify_hash(const uint8_t *data, size_t size); +int mrc_cache_verify_hash(uint32_t index, const uint8_t *data, size_t size);
#endif /* _MRC_CACHE_HASH_TPM_H_ */ diff --git a/src/security/vboot/secdata_mock.c b/src/security/vboot/secdata_mock.c index ae124da..78cb3e6 100644 --- a/src/security/vboot/secdata_mock.c +++ b/src/security/vboot/secdata_mock.c @@ -42,17 +42,17 @@ return VB2_SUCCESS; }
-vb2_error_t antirollback_lock_space_rec_hash(void) +vb2_error_t antirollback_lock_space_mrc_hash(uint32_t index) { return VB2_SUCCESS; }
-vb2_error_t antirollback_read_space_rec_hash(uint8_t *data, uint32_t size) +vb2_error_t antirollback_read_space_mrc_hash(uint32_t index, uint8_t *data, uint32_t size) { return VB2_SUCCESS; }
-vb2_error_t antirollback_write_space_rec_hash(const uint8_t *data, +vb2_error_t antirollback_write_space_mrc_hash(uint32_t index, const uint8_t *data, uint32_t size) { return VB2_SUCCESS; diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 691d2c0..451f043 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -71,10 +71,10 @@ return TPM_SUCCESS; }
-static uint32_t read_space_rec_hash(uint8_t *data) +static uint32_t read_space_mrc_hash(uint32_t index, uint8_t *data) { - RETURN_ON_FAILURE(tlcl_read(REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE)); + RETURN_ON_FAILURE(tlcl_read(index, data, + HASH_NV_SIZE)); return TPM_SUCCESS; }
@@ -83,7 +83,7 @@ * it. Since there is no data available to calculate hash at the point where TPM * space is defined, initialize it to all 0s. */ -static const uint8_t rec_hash_data[REC_HASH_NV_SIZE] = { }; +static const uint8_t mrc_hash_data[HASH_NV_SIZE] = { };
#if CONFIG(TPM2) /* @@ -162,10 +162,9 @@ VB2_SECDATA_KERNEL_SIZE, rw_space_attributes, NULL, 0); }
-static uint32_t set_rec_hash_space(const uint8_t *data) +static uint32_t set_mrc_hash_space(uint32_t index, const uint8_t *data) { - return set_space("MRC Hash", REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE, + return set_space("MRC Hash", index, data, HASH_NV_SIZE, ro_space_attributes, pcr0_unchanged_policy, sizeof(pcr0_unchanged_policy)); } @@ -185,7 +184,7 @@ RETURN_ON_FAILURE(set_kernel_space(ctx->secdata_kernel));
if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) - RETURN_ON_FAILURE(set_rec_hash_space(rec_hash_data)); + RETURN_ON_FAILURE(set_mrc_hash_space(MRC_REC_HASH_NV_INDEX, mrc_hash_data));
RETURN_ON_FAILURE(set_firmware_space(ctx->secdata_firmware));
@@ -197,9 +196,9 @@ return tlcl_lock_nv_write(FIRMWARE_NV_INDEX); }
-uint32_t antirollback_lock_space_rec_hash(void) +uint32_t antirollback_lock_space_mrc_hash(uint32_t index) { - return tlcl_lock_nv_write(REC_HASH_NV_INDEX); + return tlcl_lock_nv_write(index); }
#else @@ -239,14 +238,14 @@ } }
-static uint32_t set_rec_hash_space(const uint8_t *data) +static uint32_t set_mrc_hash_space(uint32_t index, const uint8_t *data) { - RETURN_ON_FAILURE(safe_define_space(REC_HASH_NV_INDEX, + RETURN_ON_FAILURE(safe_define_space(index, TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE, - REC_HASH_NV_SIZE)); - RETURN_ON_FAILURE(safe_write(REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE)); + HASH_NV_SIZE)); + RETURN_ON_FAILURE(safe_write(index, data, + HASH_NV_SIZE));
return TPM_SUCCESS; } @@ -307,7 +306,7 @@
/* Define and set rec hash space, if available. */ if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) - RETURN_ON_FAILURE(set_rec_hash_space(rec_hash_data)); + RETURN_ON_FAILURE(set_mrc_hash_space(MRC_REC_HASH_NV_INDEX, mrc_hash_data));
return TPM_SUCCESS; } @@ -317,7 +316,7 @@ return tlcl_set_global_lock(); }
-uint32_t antirollback_lock_space_rec_hash(void) +uint32_t antirollback_lock_space_mrc_hash(uint32_t index) { /* * Nothing needs to be done here, since global lock is already set while @@ -417,43 +416,43 @@ return safe_write(KERNEL_NV_INDEX, ctx->secdata_kernel, size); }
-uint32_t antirollback_read_space_rec_hash(uint8_t *data, uint32_t size) +uint32_t antirollback_read_space_mrc_hash(uint32_t index, uint8_t *data, uint32_t size) { - if (size != REC_HASH_NV_SIZE) { - VBDEBUG("TPM: Incorrect buffer size for rec hash. " - "(Expected=0x%x Actual=0x%x).\n", REC_HASH_NV_SIZE, + if (size != HASH_NV_SIZE) { + VBDEBUG("TPM: Incorrect buffer size for hash idx 0x%x. " + "(Expected=0x%x Actual=0x%x).\n", index, HASH_NV_SIZE, size); return TPM_E_READ_FAILURE; } - return read_space_rec_hash(data); + return read_space_mrc_hash(index, data); }
-uint32_t antirollback_write_space_rec_hash(const uint8_t *data, uint32_t size) +uint32_t antirollback_write_space_mrc_hash(uint32_t index, const uint8_t *data, uint32_t size) { - uint8_t spc_data[REC_HASH_NV_SIZE]; + uint8_t spc_data[HASH_NV_SIZE]; uint32_t rv;
- if (size != REC_HASH_NV_SIZE) { - VBDEBUG("TPM: Incorrect buffer size for rec hash. " - "(Expected=0x%x Actual=0x%x).\n", REC_HASH_NV_SIZE, + if (size != HASH_NV_SIZE) { + VBDEBUG("TPM: Incorrect buffer size for hash idx 0x%x. " + "(Expected=0x%x Actual=0x%x).\n", index, HASH_NV_SIZE, size); return TPM_E_WRITE_FAILURE; }
- rv = read_space_rec_hash(spc_data); + rv = read_space_mrc_hash(index, spc_data); if (rv == TPM_E_BADINDEX) { /* - * If space is not defined already for recovery hash, define + * If space is not defined already for hash, define * new space. */ - VBDEBUG("TPM: Initializing recovery hash space.\n"); - return set_rec_hash_space(data); + VBDEBUG("TPM: Initializing hash space.\n"); + return set_mrc_hash_space(index, data); }
if (rv != TPM_SUCCESS) return rv;
- return safe_write(REC_HASH_NV_INDEX, data, size); + return safe_write(index, data, size); }
vb2_error_t vb2ex_tpm_clear_owner(struct vb2_context *ctx) diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 0f18f9a..dbaa883 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -404,7 +404,7 @@
/* Lock rec hash space if available. */ if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) { - rv = antirollback_lock_space_rec_hash(); + rv = antirollback_lock_space_mrc_hash(MRC_REC_HASH_NV_INDEX); if (rv) { printk(BIOS_INFO, "Failed to lock rec hash space(%x)\n", rv);
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Shelley Chen (Code Review)