coreboot-gerrit October 2020

coreboot-gerrit@coreboot.org

1 participants
3474 discussions

Change in coreboot[master]: util/sconfig: allow to override chip-wrapped devices
by Michael Niewöhner (Code Review) 21 Oct '20

21 Oct '20

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46531 ) Change subject: util/sconfig: allow to override chip-wrapped devices ...................................................................... Patch Set 7: > Patch Set 7: > > > Patch Set 7: > > > > Thinking about this again, I found a case where we can't wrap the device in the chipset dt with a driver: PCIe Wifi cards. They are on random root ports and where the driver must wrap the device is decided in the boards dt, not in the chipset dt. > > In the case of PCIe WiFi, we don't wrap the root ports with the wifi drivers. Instead it is the downstream device which corresponds to the WiFi device. So, you don't need to wrap the chipset PCIe RP with wifi driver at all. Oh yes ofc! My fault.... -- To view, visit https://review.coreboot.org/c/coreboot/+/46531 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6364b3a6e1804a23503f42c66c5001e42f911270 Gerrit-Change-Number: 46531 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 21 Oct 2020 00:20:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/sconfig: allow to override chip-wrapped devices
by Furquan Shaikh (Code Review) 21 Oct '20

21 Oct '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46531 ) Change subject: util/sconfig: allow to override chip-wrapped devices ...................................................................... Patch Set 7: > Patch Set 7: > > > Patch Set 7: > > > > > Patch Set 7: > > > > > > > > I'm not sure if we actually want to allow changing the chip. > > > > > Alternatively, we could error out and fix the drivers. Some- > > > > > thing like CB:41745 might help. > > > > > > > > Well, when we turn it around (having the driver inside the device instead of wrapping the device) the problem should be gone. This is what CB:41745 does, right? > > > > > > It also adds another pointer to a config struct separate from > > > the chip's one. It wouldn't solve the problem if trees conflict, > > > but could avoid conflicting trees in some cases. > > > > I agree with Nico, this is, as things currently stand, just an error in whoever is writing the devicetrees. Technically, it is wrong to not have both devices wrapped with the same chip driver (sconfig assumes the hierarchy w/r/t devices & chips is the same in that respect) > > Just to be sure I get this right, you want to 1) have the devices wrapped by drivers in the chipset dt and 2) wrap them even when just disabling a device in a devtree? The PCI devices corresponding to the internal controllers must not be wrapped by any chip drivers. This is how almost all the internal chip devices are organized in coreboot. The only exception is the CNVi device which happened to be wrapped inconsistently. I have some patches that I plan to push this week to fix that. If there is a need to wrap a chip driver for the internal devices for any reason (ACPI generation, etc.), then that can be done by adding a virtual/dummy device under the controller device. This device wouldn't have to be modified by the mainboard unless it needs to override some chip config. Thus, the only cases where a mainboard would have to follow the chip wrapping to match the chipset tree is: 1. For SoC chip 2. For any virtual device chips that require chip config changes. -- To view, visit https://review.coreboot.org/c/coreboot/+/46531 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6364b3a6e1804a23503f42c66c5001e42f911270 Gerrit-Change-Number: 46531 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 20 Oct 2020 23:55:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/sconfig: allow to override chip-wrapped devices
by Furquan Shaikh (Code Review) 21 Oct '20

21 Oct '20

1 0

Change in coreboot[master]: soc/intel/xeon_sp: Enable SMI handler
by Jonathan Zhang (Code Review) 21 Oct '20

21 Oct '20

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46231 ) Change subject: soc/intel/xeon_sp: Enable SMI handler ...................................................................... Patch Set 13: Some comments: a. The change to pmclib.c should be split out as a patch of its own. b. Marc is working on to unify SKX-SP and CPX-SP soc code, please keep this in mind. -- To view, visit https://review.coreboot.org/c/coreboot/+/46231 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iabee5c72f0245ab988d477ac8df3d8d655a2a506 Gerrit-Change-Number: 46231 Gerrit-PatchSet: 13 Gerrit-Owner: Rocky Phagura Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Eugene Myers <cedarhouse1(a)comcast.net> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 20 Oct 2020 23:48:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mrc_cache: Remove unnecessary data checksum calculation
by Shelley Chen (Code Review) 21 Oct '20

21 Oct '20

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46515 ) Change subject: mrc_cache: Remove unnecessary data checksum calculation ...................................................................... mrc_cache: Remove unnecessary data checksum calculation When MRC_SAVE_HASH_IN_TPM is selected, we can just use the TPM hash to verify the MRC_CACHE data. Thus, we don't need to calculate the checksum anymore in this case. BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami Change-Id: I1db4469da49755805b541f50c7ef2f9cdb749425 Signed-off-by: Shelley Chen <shchen(a)google.com> --- M src/drivers/mrc_cache/mrc_cache.c 1 file changed, 10 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/46515/1 diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index 8124b24..068ce9e 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -194,18 +194,19 @@ if (md->data_size != data_size) return -1; - checksum = compute_ip_checksum(data, data_size); + if (hash_idx && CONFIG(MRC_SAVE_HASH_IN_TPM)) { + if (!mrc_cache_verify_hash(hash_idx, data, data_size)) + return -1; + } else { + checksum = compute_ip_checksum(data, data_size); - if (md->data_checksum != checksum) { - printk(BIOS_ERR, "MRC: data checksum mismatch: %x vs %x\n", - md->data_checksum, checksum); - return -1; + if (md->data_checksum != checksum) { + printk(BIOS_ERR, "MRC: data checksum mismatch: %x vs %x\n", + md->data_checksum, checksum); + return -1; + } } - if (hash_idx && CONFIG(MRC_SAVE_HASH_IN_TPM) && - !mrc_cache_verify_hash(hash_idx, data, data_size)) - return -1; - return 0; } -- To view, visit https://review.coreboot.org/c/coreboot/+/46515 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1db4469da49755805b541f50c7ef2f9cdb749425 Gerrit-Change-Number: 46515 Gerrit-PatchSet: 1 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-MessageType: newchange

3 5

Change in coreboot[master]: mrc_cache: Add tpm_hash_index field to cache_region struct
by Shelley Chen (Code Review) 21 Oct '20

21 Oct '20

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(a)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(&region, 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(&region, 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); } } -- To view, visit https://review.coreboot.org/c/coreboot/+/46514 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1a744d6f40f062ca3aab6157b3747e6c1f6977f9 Gerrit-Change-Number: 46514 Gerrit-PatchSet: 1 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-MessageType: newchange

3 12

Change in coreboot[master]: security/vboot: Add new TPM NVRAM index MRC_RW_HASH_NV_INDEX
by Shelley Chen (Code Review) 21 Oct '20

21 Oct '20

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46511 ) Change subject: security/vboot: Add new TPM NVRAM index MRC_RW_HASH_NV_INDEX ...................................................................... security/vboot: Add new TPM NVRAM index MRC_RW_HASH_NV_INDEX Add new index for MRC_CACHE data in RW. Also update antirollback functions to handle this new index where necessary. BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami Change-Id: I2de3c23aa56d3b576ca54dbd85c75e5b80199560 Signed-off-by: Shelley Chen <shchen(a)google.com> --- M src/security/vboot/antirollback.h M src/security/vboot/mrc_cache_hash_tpm.c 2 files changed, 7 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/46511/1 diff --git a/src/security/vboot/antirollback.h b/src/security/vboot/antirollback.h index 516c23e..d1309d9 100644 --- a/src/security/vboot/antirollback.h +++ b/src/security/vboot/antirollback.h @@ -24,6 +24,9 @@ #define FWMP_NV_INDEX 0x100a /* 0x100b: Hash of MRC_CACHE training data for recovery boot */ #define MRC_REC_HASH_NV_INDEX 0x100b +/* 0x100c: OOBE autoconfig public key hashes */ +/* 0x100d: Hash of MRC_CACHE training data for non-recovery boot */ +#define MRC_RW_HASH_NV_INDEX 0x100d #define HASH_NV_SIZE VB2_SHA256_DIGEST_SIZE /* Structure definitions for TPM spaces */ diff --git a/src/security/vboot/mrc_cache_hash_tpm.c b/src/security/vboot/mrc_cache_hash_tpm.c index ea67dda..144b51c 100644 --- a/src/security/vboot/mrc_cache_hash_tpm.c +++ b/src/security/vboot/mrc_cache_hash_tpm.c @@ -23,7 +23,8 @@ 0xba, 0xad, 0xba, 0xad, /* BAADBAAD */ }; const uint8_t *hash_ptr = data_hash; - uint32_t hash_idx = MRC_REC_HASH_NV_INDEX; + uint32_t hash_idx = vboot_recovery_mode_enabled() ? + MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX; /* Initialize TPM driver. */ if (tlcl_lib_init() != VB2_SUCCESS) { @@ -60,7 +61,8 @@ { uint8_t data_hash[VB2_SHA256_DIGEST_SIZE]; uint8_t tpm_hash[VB2_SHA256_DIGEST_SIZE]; - uint32_t hash_idx = MRC_REC_HASH_NV_INDEX; + uint32_t hash_idx = vboot_recovery_mode_enabled() ? + MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX; /* Calculate hash of data read from MRC_CACHE. */ if (vb2_digest_buffer(data, size, VB2_HASH_SHA256, data_hash, -- To view, visit https://review.coreboot.org/c/coreboot/+/46511 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2de3c23aa56d3b576ca54dbd85c75e5b80199560 Gerrit-Change-Number: 46511 Gerrit-PatchSet: 1 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-MessageType: newchange

3 19

Change in coreboot[master]: security/vboot: Make mrc_cache hash functions generic
by Shelley Chen (Code Review) 21 Oct '20

21 Oct '20

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(a)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); -- To view, visit https://review.coreboot.org/c/coreboot/+/46432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9c0bb25eafc731ca9c7a95113ab940f55997fc0f Gerrit-Change-Number: 46432 Gerrit-PatchSet: 1 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-MessageType: newchange

4 39

Change in coreboot[master]: mrc_cache: Move mrc_cache_*_hash functions into mrc_cache driver
by Shelley Chen (Code Review) 21 Oct '20

21 Oct '20

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46510 ) Change subject: mrc_cache: Move mrc_cache_*_hash functions into mrc_cache driver ...................................................................... mrc_cache: Move mrc_cache_*_hash functions into mrc_cache driver This CL would remove these calls from fsp 2.0. BUG=b:150502246 BRANCH=None TEST=make sure memory training still works on nami Change-Id: I3088ca6927c7dbc65386c13e868afa0462086937 Signed-off-by: Shelley Chen <shchen(a)google.com> --- M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c 2 files changed, 7 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/46510/1 diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 09aad6b..68cc121 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -19,18 +19,12 @@ #include <symbols.h> #include <timestamp.h> #include <security/vboot/vboot_common.h> -#include <security/vboot/mrc_cache_hash_tpm.h> #include <security/tpm/tspi.h> #include <vb2_api.h> #include <types.h> static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t)); -/* TPM MRC hash functionality depends on vboot starting before memory init. */ -_Static_assert(!CONFIG(MRC_SAVE_HASH_IN_TPM) || - CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), - "for TPM MRC hash functionality, vboot must start in bootblock"); - static void save_memory_training_data(bool s3wake, uint32_t fsp_version) { size_t mrc_data_size; @@ -54,9 +48,6 @@ if (mrc_cache_stash_data(MRC_TRAINING_DATA, fsp_version, mrc_data, mrc_data_size) < 0) printk(BIOS_ERR, "Failed to stash MRC data\n"); - - if (CONFIG(MRC_SAVE_HASH_IN_TPM)) - mrc_cache_update_hash(mrc_data, mrc_data_size); } static void do_fsp_post_memory_init(bool s3wake, uint32_t fsp_version) @@ -121,10 +112,6 @@ if (data == NULL) return; - if (CONFIG(MRC_SAVE_HASH_IN_TPM) && - !mrc_cache_verify_hash(data, mrc_size)) - return; - /* MRC cache found */ arch_upd->NvsBufferPtr = data; diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index a083655..30384e8 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/mrc_cache_hash_tpm.h> #include <security/vboot/vboot_common.h> #include <spi_flash.h> @@ -185,6 +186,10 @@ return -1; } + // NOTE: we need to create the hash from both data and metadata values + if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(data, data_size)) + return -1; + return 0; } @@ -443,6 +448,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)) + mrc_cache_update_hash(new_data, new_data_size); } } -- To view, visit https://review.coreboot.org/c/coreboot/+/46510 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3088ca6927c7dbc65386c13e868afa0462086937 Gerrit-Change-Number: 46510 Gerrit-PatchSet: 1 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

4 13

Change in coreboot[master]: cpu/intel/common: rework code previously moved to common cpu code
by Michael Niewöhner (Code Review) 21 Oct '20

21 Oct '20

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46588 ) Change subject: cpu/intel/common: rework code previously moved to common cpu code ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/46588/3/src/include/cpu/x86/msr.h File src/include/cpu/x86/msr.h: https://review.coreboot.org/c/coreboot/+/46588/3/src/include/cpu/x86/msr.h@… PS3, Line 24: #define CPUID_EPP (1 << 3) > ouch, good catch! it's EPB, not EPP and it's indeed cpuid 6 ecx Done -- To view, visit https://review.coreboot.org/c/coreboot/+/46588 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7615fc26625c44931577216ea42f0a733b99e131 Gerrit-Change-Number: 46588 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 20 Oct 2020 23:21:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2020