coreboot-gerrit December 2021

coreboot-gerrit@coreboot.org

1 participants
5525 discussions

Change in coreboot[master]: soc/intel/common/block/pcie/rtd3: Add ModPHY power gate support for RTD3
by Subrata Banik (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Cliff Huang, Maulik V Vaghela, Tim Wawrzynczak, Patrick Rudolph. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59855 ) Change subject: soc/intel/common/block/pcie/rtd3: Add ModPHY power gate support for RTD3 ...................................................................... Patch Set 3: (4 comments) File src/soc/intel/common/block/pcie/rtd3/rtd3.c: https://review.coreboot.org/c/coreboot/+/59855/comment/0ed5906c_a85bd27a PS2, Line 238: static bool mutex_created; > Done Why we need to initialize to `false` ? Default value itself is false. Isn't it? #include <stdio.h> #include <stdbool.h> int main() { static bool a; bool b; printf("Default value of static variable : %s\n", a == true ? "true" : "false"); printf("Default value of non-static variable : %s\n", b == true ? "true" : "false"); return 0; } $gcc -o main *.c -lm $main Default value of static variable : false Default value of non-static variable : false File src/soc/intel/common/block/pcie/rtd3/rtd3.c: https://review.coreboot.org/c/coreboot/+/59855/comment/e896e1f0_e0af4d3e PS3, Line 83: bool enabl Just thoughts: Do you really need line #90 if/else clause ? can't we have like this? enum modphy_pg_state { PG_DISABLE = 0, PG_ENABLE = 1, }; static void pcie_rtd3_enable_modphy_pg(unsigned int pcie_rp, enum modphy_pg_state state) { /* Enter the critical section */ .... acpigen_write_store_int_to_namestr(state, "EMPG"); acpigen_write_delay_until_namestr_int(100, "AMPG", state); /* Exit the critical section */ .... } https://review.coreboot.org/c/coreboot/+/59855/comment/cc697db2_a9f21973 PS3, Line 112: pcie_rtd3_enable_modphy_pg(pcie_rp, false); pcie_rtd3_enable_modphy_pg(pcie_rp, PG_DISABLE); https://review.coreboot.org/c/coreboot/+/59855/comment/bc3138ec_221bcec2 PS3, Line 159: pcie_rtd3_enable_modphy_pg(pcie_rp, true); pcie_rtd3_enable_modphy_pg(pcie_rp, PG_ENABLE); -- To view, visit https://review.coreboot.org/c/coreboot/+/59855 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I19cb05a74acfa3ded7867b1cac32c161a83b4f7d Gerrit-Change-Number: 59855 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 08 Dec 2021 19:16:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Documentation/releases: Update 4.16 release notes
by Sean Rhodes (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Angel Pons. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59564 ) Change subject: Documentation/releases: Update 4.16 release notes ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Hey, any chance you could review this please? -- To view, visit https://review.coreboot.org/c/coreboot/+/59564 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9675a6a8960d93ae6de285d8b25ffc48a763483e Gerrit-Change-Number: 59564 Gerrit-PatchSet: 1 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 08 Dec 2021 19:04:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush/var/nipperkin: Configure Smart Card in normal mode
by Karthik Ramasubramanian (Code Review) 08 Dec '21

08 Dec '21

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59984 ) Change subject: mb/google/guybrush/var/nipperkin: Configure Smart Card in normal mode ...................................................................... mb/google/guybrush/var/nipperkin: Configure Smart Card in normal mode As per the schematics, smart card is expected to operate in normal mode by default. So configure the SOC_SC_PWRSV gpio accordingly. BUG=b:202992077 TEST=Build and boot to OS in Nipperkin board version 2. Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: I8e12600ad45734b144a30c868f0e4f323aa056f6 --- M src/mainboard/google/guybrush/variants/nipperkin/gpio.c 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/59984/1 diff --git a/src/mainboard/google/guybrush/variants/nipperkin/gpio.c b/src/mainboard/google/guybrush/variants/nipperkin/gpio.c index 887be83..df55e52 100644 --- a/src/mainboard/google/guybrush/variants/nipperkin/gpio.c +++ b/src/mainboard/google/guybrush/variants/nipperkin/gpio.c @@ -36,6 +36,8 @@ PAD_NC(GPIO_17), /* LCD_PRIVACY_PCH */ PAD_GPO(GPIO_18, HIGH), + /* SOC_SC_PWRSV */ + PAD_GPO(GPIO_31, HIGH), }; static const struct soc_amd_gpio override_early_gpio_table[] = { -- To view, visit https://review.coreboot.org/c/coreboot/+/59984 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8e12600ad45734b144a30c868f0e4f323aa056f6 Gerrit-Change-Number: 59984 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: mb/google/guybrush/var/nipperkin: Override SPI fast speed
by Karthik Ramasubramanian (Code Review) 08 Dec '21

08 Dec '21

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59983 ) Change subject: mb/google/guybrush/var/nipperkin: Override SPI fast speed ...................................................................... mb/google/guybrush/var/nipperkin: Override SPI fast speed After assessing the signal integrity, 100 MHz SPI fast speed can be enabled for SPI ROM. BUG=None TEST=Build and boot to OS in Nipperkin board version 2. Perform 250 iterations of warm and cold reset each. Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: Id973acb939b69e0beda26252e57a278892f2f57d --- M src/mainboard/google/guybrush/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/59983/1 diff --git a/src/mainboard/google/guybrush/Kconfig b/src/mainboard/google/guybrush/Kconfig index c97c06d..7d9cd3d 100644 --- a/src/mainboard/google/guybrush/Kconfig +++ b/src/mainboard/google/guybrush/Kconfig @@ -123,6 +123,7 @@ config OVERRIDE_EFS_SPI_SPEED_MIN_BOARD hex default 0x4 if BOARD_GOOGLE_GUYBRUSH + default 0x2 if BOARD_GOOGLE_NIPPERKIN default 0xffffffff help Minimum board version starting which the Override EFS SPI Speed -- To view, visit https://review.coreboot.org/c/coreboot/+/59983 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id973acb939b69e0beda26252e57a278892f2f57d Gerrit-Change-Number: 59983 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: cbfs: Add verification for RO CBFS master hash
by Julius Werner (Code Review) 08 Dec '21

08 Dec '21

Hello Patrick Georgi, Aaron Durbin, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/41120 to review the following change. Change subject: cbfs: Add verification for RO CBFS master hash ...................................................................... cbfs: Add verification for RO CBFS master hash This patch adds the first stage of the new CONFIG_CBFS_VERIFICATION feature. It's not useful to end-users in this stage so it cannot be selected in menuconfig (and should not be used other than for development) yet. With this patch coreboot can verify the master hash of the RO CBFS when it starts booting, but it does not verify individual files yet. Likewise, verifying RW CBFSes with vboot is not yet supported. Verification is bootstrapped from a "master hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS master hash and a separate hash for the FMAP which is required to find the primary CBFS. Both are verified on first use in the bootblock (and halt the system on failure). The CONFIG_TOCTOU_SAFETY option is also added for illustrative purposes to show some paths that need to be different when full protection against TOCTOU (time-of-check vs. time-of-use) attacks is desired. For normal verification it is sufficient to check the FMAP and the CBFS master hash only once in the bootblock -- for TOCTOU verification we do the same, but we need to be extra careful that we do not re-read the FMAP or any CBFS metadata in later stages. This is mostly achieved by depending on the CBFS metadata cache and FMAP cache features, but we allow for one edge case in case the RW CBFS metadata cache overflows (which may happen during an RW update and could otherwise no longer be fixed because mcache size is defined by RO code). This code is added to demonstrate design intent but won't really matter until RW CBFS verification can be supported. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/master_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/master_hash.h A src/lib/Kconfig.cbfs_verification M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c A src/lib/master_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c 13 files changed, 218 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/1 diff --git a/src/commonlib/bsd/include/commonlib/bsd/master_hash.h b/src/commonlib/bsd/include/commonlib/bsd/master_hash.h new file mode 100644 index 0000000..f8bad6c --- /dev/null +++ b/src/commonlib/bsd/include/commonlib/bsd/master_hash.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-only */ + +#ifndef _COMMONLIB_BSD_MASTER_HASH_H_ +#define _COMMONLIB_BSD_MASTER_HASH_H_ + +#include <stdint.h> +#include <vb2_sha.h> + +/* This structure is embedded somewhere in the (uncompressed) bootblock. */ +struct master_hash_anchor { + uint8_t magic[8]; + struct vb2_hash cbfs_hash; + /* NOTE: This is just reserving space. The FMAP hash is placed right after the CBFS + hash above, which may be shorter than the compile-time size of struct vb2_hash! */ + uint8_t reserved_space_for_fmap_hash[VB2_MAX_DIGEST_SIZE]; +} __packed; + +/* Always use this function to figure out the actual location of the FMAP hash. It always uses + the same algorithm as the CBFS hash. */ +static inline uint8_t *master_hash_anchor_fmap_hash(struct master_hash_anchor *anchor) +{ + return anchor->cbfs_hash.raw + vb2_digest_size(anchor->cbfs_hash.algo); +} + +/* Must *not* appear anywhere else in coreboot code (other than the anchor). */ +#define MASTER_HASH_ANCHOR_MAGIC "\xadMstHsh\x15" + +#endif /* _COMMONLIB_BSD_MASTER_HASH_H_ */ diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 7505fb2..710fd3b 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -25,6 +25,7 @@ #define CBMEM_ID_IGD_OPREGION 0x4f444749 #define CBMEM_ID_IMD_ROOT 0xff4017ff #define CBMEM_ID_IMD_SMALL 0x53a11439 +#define CBMEM_ID_MASTER_HASH 0x6873484D #define CBMEM_ID_MEMINFO 0x494D454D #define CBMEM_ID_MMA_DATA 0x4D4D4144 #define CBMEM_ID_MMC_STATUS 0x4d4d4353 diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 66cbe21..da3b80a 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -7,6 +7,7 @@ #include <commonlib/cbfs.h> #include <program_loading.h> #include <types.h> +#include <vb2_sha.h> /*********************************************** * Perform CBFS operations on the boot device. * @@ -65,4 +66,13 @@ */ const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro); +/* + * Builds the mcache (if |cbd->mcache| is set) and verifies the master hash (if + * |master_hash| is not NULL). If CB_CBFS_CACHE_FULL is returned, the mcache is + * incomplete but still valid and the master hash was still verified. Should be + * called once per *boot* (not once per stage) before the first CBFS access. + */ +cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd, + struct vb2_hash *master_hash); + #endif diff --git a/src/include/cbfs_glue.h b/src/include/cbfs_glue.h index 7c40714..7836e72 100644 --- a/src/include/cbfs_glue.h +++ b/src/include/cbfs_glue.h @@ -6,7 +6,7 @@ #include <commonlib/region.h> #include <console/console.h> -#define CBFS_ENABLE_HASHING 0 +#define CBFS_ENABLE_HASHING CONFIG(CBFS_VERIFICATION) #define ERROR(...) printk(BIOS_ERR, "CBFS ERROR: " __VA_ARGS__) #define LOG(...) printk(BIOS_ERR, "CBFS: " __VA_ARGS__) diff --git a/src/include/master_hash.h b/src/include/master_hash.h new file mode 100644 index 0000000..2ce1521 --- /dev/null +++ b/src/include/master_hash.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef _MASTER_HASH_H_ +#define _MASTER_HASH_H_ + +#include <commonlib/bsd/master_hash.h> + +vb2_error_t master_hash_verify_fmap(const void *fmap_base, size_t fmap_size); + +#if CONFIG(CBFS_VERIFICATION) +struct vb2_hash *master_hash_get(void); +#else +static inline struct vb2_hash *master_hash_get(void) { return NULL; } +#endif + +#endif diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification new file mode 100644 index 0000000..c11f1af --- /dev/null +++ b/src/lib/Kconfig.cbfs_verification @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-or-later +# +# This file is part of the coreboot project. +# +# This file is sourced from src/security/Kconfig for menuconfig convenience. + +#menu "CBFS verification" # TODO: enable once it works + +config CBFS_VERIFICATION + bool # TODO: make user selectable once it works + depends on !COMPRESS_BOOTBLOCK # TODO: figure out decompressor anchor + help + Work in progress. Do not use (yet). + +config TOCTOU_SAFETY + bool + depends on CBFS_VERIFICATION + depends on !NO_FMAP_CACHE + depends on !NO_CBFS_MCACHE + help + Work in progress. Not actually TOCTOU safe yet. Do not use. + + Design idea here is that mcache overflows in this mode are only legal + for the RW CBFS, because it's relatively easy to retrieve the RW + master hash from persistent vboot context at any time, but the RO + master hash is lost after the bootblock is unloaded. This avoids the + need to carry yet another piece forward through the stages. Mcache + overflows are mostly a concern for RW updates (if an update adds more + files than originally planned for), for the RO section it should + always be possible to dimension the mcache correctly beforehand, so + this should be an acceptable limitation. + +config CBFS_HASH_ALGO + int + default 1 if CBFS_HASH_SHA1 + default 2 if CBFS_HASH_SHA256 + default 3 if CBFS_HASH_SHA512 + +choice + prompt "--> hash type" + depends on CBFS_VERIFICATION + default CBFS_HASH_SHA256 + +config CBFS_HASH_SHA1 + bool "SHA-1" + +config CBFS_HASH_SHA256 + bool "SHA-256" + +config CBFS_HASH_SHA512 + bool "SHA-512" + +endchoice + +#endmenu diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 085f6b2..a031c01a 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -37,6 +37,7 @@ bootblock-y += cbfs.c bootblock-$(CONFIG_GENERIC_GPIO_LIB) += gpio.c bootblock-y += libgcc.c +bootblock-$(CONFIG_CBFS_VERIFICATION) += master_hash.c bootblock-$(CONFIG_GENERIC_UDELAY) += timer.c bootblock-$(CONFIG_COLLECT_TIMESTAMPS) += timestamp.c diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 201e705..992d104 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -11,6 +11,7 @@ #include <endian.h> #include <fmap.h> #include <lib.h> +#include <master_hash.h> #include <security/tpm/tspi/crtm.h> #include <security/vboot/vboot_common.h> #include <stdlib.h> @@ -30,8 +31,20 @@ if (!CONFIG(NO_CBFS_MCACHE)) err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size, name, mdata, &data_offset); - if (err == CB_CBFS_CACHE_FULL) - err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, NULL); + if (err == CB_CBFS_CACHE_FULL) { + struct vb2_hash *master_hash = NULL; + if (CONFIG(TOCTOU_SAFETY)) { + if (cbd != vboot_get_cbfs_boot_device()) { + printk(BIOS_ERR, + "ERROR: RO mcache overflow for %s breaks TOCTOU safety!\n", + name); + return CB_CBFS_CACHE_FULL; + } + /* TODO: set master_hash to RW master hash here. */ + } + err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, + master_hash); + } if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro && err == CB_CBFS_NOT_FOUND) { @@ -308,6 +321,26 @@ return 0; } +cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd, + struct vb2_hash *master_hash) +{ + /* If we have an mcache, mcache_build() will also check master hash. */ + if (!CONFIG(NO_CBFS_MCACHE) && cbd->mcache_size > 0) + return cbfs_mcache_build(&cbd->rdev, cbd->mcache, + cbd->mcache_size, master_hash); + + /* No mcache and no verification means we have nothing special to do. */ + if (!CONFIG(CBFS_VERIFICATION) || !master_hash) + return CB_SUCCESS; + + /* Verification only: use cbfs_walk() without a walker() function to + just run through the CBFS once, will return NOT_FOUND by default. */ + cb_err_t err = cbfs_walk(&cbd->rdev, NULL, NULL, master_hash, 0); + if (err == CB_CBFS_NOT_FOUND) + err = CB_SUCCESS; + return err; +} + const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro) { static struct cbfs_boot_device ro; @@ -328,7 +361,7 @@ return &ro; if (fmap_locate_area_as_rdev("COREBOOT", &ro.rdev)) - return NULL; + die("Cannot locate primary CBFS"); if (!CONFIG(NO_CBFS_MCACHE)) { const struct cbmem_entry *entry; @@ -341,12 +374,14 @@ ro.mcache_size = REGION_SIZE(cbfs_mcache) * (100 - CONFIG_CBFS_MCACHE_RW_PERCENTAGE) / 100; } - if (ENV_BOOTBLOCK) { - cb_err_t err = cbfs_mcache_build(&ro.rdev, ro.mcache, - ro.mcache_size, NULL); - if (err && err != CB_CBFS_CACHE_FULL) - die("Failed to build RO mcache"); - } + } + + if (ENV_BOOTBLOCK) { + cb_err_t err = cbfs_init_boot_device(&ro, master_hash_get()); + if (err == CB_CBFS_HASH_MISMATCH) + die("RO CBFS master hash verification failure"); + else if (err && err != CB_CBFS_CACHE_FULL) + die("RO CBFS initialization error: %d", err); } return &ro; diff --git a/src/lib/fmap.c b/src/lib/fmap.c index ecd23f6..197ad0e 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -5,6 +5,7 @@ #include <cbmem.h> #include <console/console.h> #include <fmap.h> +#include <master_hash.h> #include <stddef.h> #include <string.h> #include <symbols.h> @@ -28,9 +29,20 @@ return FMAP_OFFSET; } -static int check_signature(const struct fmap *fmap) +static int verify_fmap(const struct fmap *fmap) { - return memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature)); + if (memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature))) + return -1; + + static bool done = false; + if (!CONFIG(CBFS_VERIFICATION) || !ENV_BOOTBLOCK || done) + return 0; /* Only need to check hash in first stage. */ + + if (master_hash_verify_fmap(fmap, FMAP_SIZE) != VB2_SUCCESS) + return -1; + + done = true; + return 0; } static void report(const struct fmap *fmap) @@ -61,10 +73,12 @@ /* NOTE: This assumes that for all platforms running this code, the bootblock is the first stage and the bootblock will make at least one FMAP access (usually from finding CBFS). */ - if (!check_signature(fmap)) + if (!verify_fmap(fmap)) goto register_cache; printk(BIOS_ERR, "ERROR: FMAP cache corrupted?!\n"); + if (CONFIG(TOCTOU_SAFETY)) + die("TOCTOU safety relies on FMAP cache"); } /* In case we fail below, make sure the cache is invalid. */ @@ -78,7 +92,7 @@ /* memlayout statically guarantees that the FMAP_CACHE is big enough. */ if (rdev_readat(boot_rdev, fmap, FMAP_OFFSET, FMAP_SIZE) != FMAP_SIZE) return; - if (check_signature(fmap)) + if (verify_fmap(fmap)) return; report(fmap); @@ -109,8 +123,9 @@ if (fmap == NULL) return -1; - if (check_signature(fmap)) { - printk(BIOS_DEBUG, "No FMAP found at %zx offset.\n", offset); + if (verify_fmap(fmap)) { + printk(BIOS_ERR, "FMAP missing or corrupted at offset 0x%zx!\n", + offset); rdev_munmap(boot, fmap); return -1; } diff --git a/src/lib/master_hash.c b/src/lib/master_hash.c new file mode 100644 index 0000000..a4b5943 --- /dev/null +++ b/src/lib/master_hash.c @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <assert.h> +#include <cbmem.h> +#include <master_hash.h> +#include <symbols.h> + +__attribute__((used, section(".master_hash_anchor"))) +struct master_hash_anchor master_hash_anchor = { + .magic = MASTER_HASH_ANCHOR_MAGIC, + .cbfs_hash = { .algo = CONFIG_CBFS_HASH_ALGO } +}; + +struct vb2_hash *master_hash_get(void) +{ + return &master_hash_anchor.cbfs_hash; +} + +vb2_error_t master_hash_verify_fmap(const void *fmap_buffer, size_t fmap_size) +{ + struct vb2_hash hash = { .algo = master_hash_anchor.cbfs_hash.algo }; + memcpy(hash.raw, master_hash_anchor_fmap_hash(&master_hash_anchor), + vb2_digest_size(hash.algo)); + return vb2_hash_verify(fmap_buffer, fmap_size, &hash); +} diff --git a/src/lib/program.ld b/src/lib/program.ld index 6f096dc..cb753e8 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -28,6 +28,7 @@ CONFIG(ARCH_BOOTBLOCK_X86_64)) KEEP(*(.id)); #endif + KEEP(*(.master_hash_anchor)); *(.text); *(.text.*); diff --git a/src/security/Kconfig b/src/security/Kconfig index 65d2def..8987369 100644 --- a/src/security/Kconfig +++ b/src/security/Kconfig @@ -11,6 +11,10 @@ ## GNU General Public License for more details. ## +# These features are implemented in src/lib/cbfs.c, but they are security +# features so sort them in here for menuconfig. +source "src/lib/Kconfig.cbfs_verification" + source "src/security/vboot/Kconfig" source "src/security/tpm/Kconfig" source "src/security/memory/Kconfig" diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index e6ac5cf..f3b1c7c 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -25,18 +25,17 @@ int vboot_executed; -static void build_rw_mcache(void) +static void after_verstage(void) { - if (CONFIG(NO_CBFS_MCACHE)) - return; + vboot_executed = 1; /* Mark verstage execution complete. */ const struct cbfs_boot_device *cbd = vboot_get_cbfs_boot_device(); - if (!cbd) /* Don't build RW mcache in recovery mode. */ + if (!cbd) /* Can't initialize RW CBFS in recovery mode. */ return; - cb_err_t err = cbfs_mcache_build(&cbd->rdev, cbd->mcache, - cbd->mcache_size, NULL); - if (err && err != CB_CBFS_CACHE_FULL) - die("Failed to build RW mcache."); /* TODO: -> recovery? */ + + cb_err_t err = cbfs_init_boot_device(cbd, NULL); /* TODO: RW hash */ + if (err && err != CB_CBFS_CACHE_FULL) /* TODO: -> recovery? */ + die("RW CBFS initialization failure: %d", err); } void vboot_run_logic(void) @@ -44,8 +43,7 @@ if (verification_should_run()) { /* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */ verstage_main(); - vboot_executed = 1; - build_rw_mcache(); + after_verstage(); } else if (verstage_should_load()) { struct cbfsf file; struct prog verstage = @@ -72,8 +70,7 @@ if (!CONFIG(VBOOT_RETURN_FROM_VERSTAGE)) return; - vboot_executed = 1; - build_rw_mcache(); + after_verstage(); } } -- To view, visit https://review.coreboot.org/c/coreboot/+/41120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 Gerrit-Change-Number: 41120 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 18

Change in coreboot[master]: cbfs: Enable CBFS verification Kconfigs
by Julius Werner (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Jakub Czapiga. Hello Jakub Czapiga, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/59982 to review the following change. Change subject: cbfs: Enable CBFS verification Kconfigs ...................................................................... cbfs: Enable CBFS verification Kconfigs With the elimination of remaining non-verifying CBFS APIs in CB:59682, CBFS verification is now ready to be used in its simplest form, so enable the respective Kconfig options in menuconfig. Add a few more restrictions to the TOCTOU_SAFETY option for problems that haven't been solved yet, and transform a comment in cbfs.c into a die() to make sure we don't accidentally forget implementing it once vboot integration gets added. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ifeba5c962c943856ab79bc6c4cb90a60c1de4a60 --- M src/lib/Kconfig.cbfs_verification M src/lib/cbfs.c 2 files changed, 33 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/59982/1 diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification index fa90d9d..33e5458 100644 --- a/src/lib/Kconfig.cbfs_verification +++ b/src/lib/Kconfig.cbfs_verification @@ -2,33 +2,41 @@ # # This file is sourced from src/security/Kconfig for menuconfig convenience. -#menu "CBFS verification" # TODO: enable once it works +menu "CBFS verification" config CBFS_VERIFICATION - bool # TODO: make user selectable once it works + bool "Enable CBFS verification" depends on !VBOOT_STARTS_BEFORE_BOOTBLOCK # this is gonna get tricky... select VBOOT_LIB help - Work in progress. Do not use (yet). + Say yes here to enable code that cryptographically verifies each CBFS + file as it gets loaded by chaining it to a trust anchor that is + embedded in the bootblock. This only makes sense if you use some + out-of-band mechanism to guarantee the integrity of the bootblock + itself, such as Intel BootGuard or flash write-protection. + + If a CBFS image was created with this option enabled, cbfstool will + automatically update the hash embedded in the bootblock whenever it + modifies the CBFS. + +if CBFS_VERIFICATION config TOCTOU_SAFETY - bool - depends on CBFS_VERIFICATION + bool "Protect against time-of-check vs. time-of-use vulnerabilities" depends on !NO_FMAP_CACHE depends on !NO_CBFS_MCACHE depends on !USE_OPTION_TABLE && !FSP_CAR # Known to access CBFS before CBMEM init + depends on !VBOOT # TODO: can only allow this once vboot fully integrated + depends on NO_XIP_EARLY_STAGES help - Work in progress. Not actually TOCTOU safe yet. Do not use. + Say yes here to eliminate time-of-check vs. time-of-use vulnerabilities + for CBFS verification. This means that data from flash must be verified + every time it is loaded (not just the first time), which requires a bit + more overhead and is incompatible with certain configurations. - Design idea here is that mcache overflows in this mode are only legal - for the RW CBFS, because it's relatively easy to retrieve the RW - metadata hash from persistent vboot context at any time, but the RO - metadata hash is lost after the bootblock is unloaded. This avoids the - need to carry yet another piece forward through the stages. Mcache - overflows are mostly a concern for RW updates (if an update adds more - files than originally planned for), for the RO section it should - always be possible to dimension the mcache correctly beforehand, so - this should be an acceptable limitation. + Using this option only makes sense when the mechanism securing the + bootblock is also safe against these vulnerabilities (i.e. there's no + point in enabling this when you just rely on flash write-protection). config CBFS_HASH_ALGO int @@ -37,9 +45,13 @@ default 3 if CBFS_HASH_SHA512 choice - prompt "--> hash type" - depends on CBFS_VERIFICATION + prompt "Hash algorithm" default CBFS_HASH_SHA256 + help + Select the hash algorithm used in CBFS verification. Note that SHA-1 is + generally considered insecure today and should not be used without good + reason. When using CBFS verification together with measured boot, using + the same hash algorithm (usually SHA-256) for both is more efficient. config CBFS_HASH_SHA1 bool "SHA-1" @@ -52,4 +64,6 @@ endchoice -#endmenu +endif + +endmenu diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 991c9fe..3a044f7 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -57,7 +57,7 @@ RO CBFS would have been caught when building the mcache in cbfs_get boot_device(). (Note that TOCTOU_SAFETY implies !NO_CBFS_MCACHE.) */ assert(cbd == vboot_get_cbfs_boot_device()); - /* TODO: set metadata_hash to RW metadata hash here. */ + die("TODO: set metadata_hash to RW metadata hash here.\n"); } err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, metadata_hash); } -- To view, visit https://review.coreboot.org/c/coreboot/+/59982 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifeba5c962c943856ab79bc6c4cb90a60c1de4a60 Gerrit-Change-Number: 59982 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: mb/intel/adlrvp: Add support for external clock buffer
by Tim Wawrzynczak (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: V Sowmya, Subrata Banik, Maulik V Vaghela, Angel Pons, Tim Wawrzynczak, EricR Lai. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59976 ) Change subject: mb/intel/adlrvp: Add support for external clock buffer ...................................................................... Patch Set 10: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59976 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I21f1155374049c90aa45db25d4128b39aa5898bb Gerrit-Change-Number: 59976 Gerrit-PatchSet: 10 Gerrit-Owner: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: V Sowmya <v.sowmya(a)intel.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 08 Dec 2021 18:10:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/stoneyridge/southbridge: drop ENV_X86 check
by Felix Held (Code Review) 08 Dec '21

08 Dec '21

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59960 ) Change subject: soc/amd/stoneyridge/southbridge: drop ENV_X86 check ...................................................................... soc/amd/stoneyridge/southbridge: drop ENV_X86 check Stoneyridge selects ARCH_X86 unconditionally and all coreboot code will run on the x86 cores. On Picasso and later, the Chromebooks run verstage on the PSP which is an ARM V7 core which needs some special handling cases in the code, but this doesn't apply to Stoneyridge. TEST=Timeless build results in an identical image for Google/Careena. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I013efd13b56c0191af034a8c4b58e9b26a31c6e9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/59960 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd(a)gmail.com> --- M src/soc/amd/stoneyridge/southbridge.c 1 file changed, 1 insertion(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index 63bfd83..3b2cba6 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -230,8 +230,7 @@ static void sb_init_spi_base(void) { /* Make sure the base address is predictable */ - if (ENV_X86) - lpc_set_spibase(SPI_BASE_ADDRESS); + lpc_set_spibase(SPI_BASE_ADDRESS); lpc_enable_spi_rom(SPI_ROM_ENABLE); } -- To view, visit https://review.coreboot.org/c/coreboot/+/59960 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I013efd13b56c0191af034a8c4b58e9b26a31c6e9 Gerrit-Change-Number: 59960 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/amd/{cezanne,picasso,stoney}: Clear PM/GPE when enabling ACPI
by Felix Held (Code Review) 08 Dec '21

08 Dec '21

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59928 ) Change subject: soc/amd/{cezanne,picasso,stoney}: Clear PM/GPE when enabling ACPI ...................................................................... soc/amd/{cezanne,picasso,stoney}: Clear PM/GPE when enabling ACPI According to https://uefi.org/specs/ACPI/6.4/16_Waking_and_Sleeping/sleeping-states.html… > For ACPI/legacy systems, when transitioning from the legacy to the G0 > working state this register is cleared by platform firmware prior to > setting the SCI_EN bit. This change makes sure we clear the PM/GPE blocks are cleared before enabling the SCI_EN bit. BUG=b:172021431 TEST=Boot guybrush and morphius to OS and verify suspend resume still works. Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: Icc6f542185dc520f8d181423961b74481c0b5506 Reviewed-on: https://review.coreboot.org/c/coreboot/+/59928 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Held <felix-coreboot(a)felixheld.de> --- M src/soc/amd/cezanne/smihandler.c M src/soc/amd/picasso/smihandler.c M src/soc/amd/stoneyridge/smihandler.c 3 files changed, 3 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved diff --git a/src/soc/amd/cezanne/smihandler.c b/src/soc/amd/cezanne/smihandler.c index 7653836..2549ef8 100644 --- a/src/soc/amd/cezanne/smihandler.c +++ b/src/soc/amd/cezanne/smihandler.c @@ -23,6 +23,7 @@ switch (cmd) { case APM_CNT_ACPI_ENABLE: + acpi_clear_pm_gpe_status(); acpi_enable_sci(); break; case APM_CNT_ACPI_DISABLE: diff --git a/src/soc/amd/picasso/smihandler.c b/src/soc/amd/picasso/smihandler.c index b13c6e0..f9f5fc0 100644 --- a/src/soc/amd/picasso/smihandler.c +++ b/src/soc/amd/picasso/smihandler.c @@ -23,6 +23,7 @@ switch (cmd) { case APM_CNT_ACPI_ENABLE: + acpi_clear_pm_gpe_status(); acpi_enable_sci(); break; case APM_CNT_ACPI_DISABLE: diff --git a/src/soc/amd/stoneyridge/smihandler.c b/src/soc/amd/stoneyridge/smihandler.c index f9c4a54..50a5111 100644 --- a/src/soc/amd/stoneyridge/smihandler.c +++ b/src/soc/amd/stoneyridge/smihandler.c @@ -21,6 +21,7 @@ switch (cmd) { case APM_CNT_ACPI_ENABLE: + acpi_clear_pm_gpe_status(); acpi_enable_sci(); break; case APM_CNT_ACPI_DISABLE: -- To view, visit https://review.coreboot.org/c/coreboot/+/59928 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc6f542185dc520f8d181423961b74481c0b5506 Gerrit-Change-Number: 59928 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: mb/intel/adlrvp: Add support for external clock buffer
by Subrata Banik (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: V Sowmya, Maulik V Vaghela, Angel Pons, Subrata Banik, Tim Wawrzynczak, EricR Lai. Hello V Sowmya, build bot (Jenkins), Maulik V Vaghela, Tim Wawrzynczak, Angel Pons, Tim Wawrzynczak, EricR Lai, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59976 to look at the new patch set (#10). Change subject: mb/intel/adlrvp: Add support for external clock buffer ...................................................................... mb/intel/adlrvp: Add support for external clock buffer ADL-P silicon can support 7 SRC CLK's and 10 CLKREQ signals. Out of 7 SRCCLK's 3 will be used for CPU, the rest are for PCH. If more than 4 PCH devices are connected on the platform, an external differential buffer chip needs to be placed at the platform level. A mainboard designer can choose to add an external clock chip, and select the SRC CLK using CONFIG_CLKSRC_FOR_EXTERNAL_BUFFER. CONFIG_CLKSRC_FOR_EXTERNAL_BUFFER provides the CLKSRC that feed clock to discrete buffer for further distribution to platform. TEST=Able to detect SD card connected at x4 PCIe Gen 3 Slot. localhost ~ # dmesg | grep mmc [ 4.997840] mmc0: SDHCI controller on PCI [0000:ae:00.0] using ADMA [ 5.460902] mmc0: new ultra high speed DDR50 SDHC card at address aaaa [ 5.473555] mmcblk0: mmc0:aaaa SS08G 7.40 GiB [ 5.494268] mmcblk0: p1 Change-Id: I21f1155374049c90aa45db25d4128b39aa5898bb Signed-off-by: Subrata Banik <subi.banik(a)gmail.com> --- M src/mainboard/intel/adlrvp/Kconfig M src/mainboard/intel/adlrvp/romstage_fsp_params.c 2 files changed, 39 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/59976/10 -- To view, visit https://review.coreboot.org/c/coreboot/+/59976 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I21f1155374049c90aa45db25d4128b39aa5898bb Gerrit-Change-Number: 59976 Gerrit-PatchSet: 10 Gerrit-Owner: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: V Sowmya <v.sowmya(a)intel.com> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2021