coreboot-gerrit June 2020

coreboot-gerrit@coreboot.org

1 participants
2826 discussions

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]: Documentation/lint: Use Super I/O instead of SuperIO
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39451 ) Change subject: Documentation/lint: Use Super I/O instead of SuperIO ...................................................................... Documentation/lint: Use Super I/O instead of SuperIO Change-Id: Idb16092b687ebffb319bc1908f08f350d612d36a Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- M Documentation/superio/nuvoton/nct5539d.md M util/lint/spelling.txt 2 files changed, 4 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/39451/1 diff --git a/Documentation/superio/nuvoton/nct5539d.md b/Documentation/superio/nuvoton/nct5539d.md index e91ebc3..009d329 100644 --- a/Documentation/superio/nuvoton/nct5539d.md +++ b/Documentation/superio/nuvoton/nct5539d.md @@ -1,9 +1,9 @@ -# NCT5539D SuperIO +# NCT5539D Super I/O -The SuperIO has the ID `0xd121` and the source can be found in +The Super I/O has the ID `0xd121` and the source can be found in `src/superio/nuvoton/nct5539d/`. ## For developers -The SuperIO generates ACPI using the +The Super I/O generates ACPI using the [SSDT generator for generic SuperIOs](../common/ssdt.md). diff --git a/util/lint/spelling.txt b/util/lint/spelling.txt index 1263144..f74abf1 100644 --- a/util/lint/spelling.txt +++ b/util/lint/spelling.txt @@ -16,6 +16,7 @@ FTBS||FTBFS POSIX-complient||POSIX-compliant READEME||README +SuperIO||Super I/O aaccessibility||accessibility aaccession||accession abailable||available -- To view, visit https://review.coreboot.org/c/coreboot/+/39451 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idb16092b687ebffb319bc1908f08f350d612d36a Gerrit-Change-Number: 39451 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 7

Change in coreboot[master]: Documentation: Add proposal for firmware testing
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37643 ) Change subject: Documentation: Add proposal for firmware testing ...................................................................... Documentation: Add proposal for firmware testing Change-Id: Icb9380050f8ff1aa13ecbb501079e2556e43ca06 Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- A Documentation/technotes/2019-12-firmware-testing.md M Documentation/technotes/index.md 2 files changed, 176 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/37643/1 diff --git a/Documentation/technotes/2019-12-firmware-testing.md b/Documentation/technotes/2019-12-firmware-testing.md new file mode 100644 index 0000000..64838a7 --- /dev/null +++ b/Documentation/technotes/2019-12-firmware-testing.md @@ -0,0 +1,175 @@ +# Draft design of a firmware-aware integration test system + +## Problem statement + +Most full system test systems have assumptions baked in that make them +unsuitable for firmware level testing. For example, both Chrome OS test +runners assume that the DUT (Device Under Test) has an operational +kernel + network + sshd. That's a useful assumption when covering +userland testing, but not for firmware. Lava has its downsides as well +(TODO: as reported by Philipp, need details). + +This document outlines a system that, when implemented, helps +automatically test properties of firmware level code. It's _likely_ +also suitable for later stages (kernel and userland), but they're +not the main concern: there's enough tooling out there for them. + +## Collecting requirements + +This project doesn't consider a test system running only on the DUT +so it will require a test host that remains in control. Apart from +that, the system should be useful at all ends of the problem space: +hobbyists with little money to spare just the same as test labs +running automated continuous testing. + +Beyond DUT and test host, the system requires some way for them to +communicate, so we'll expect a bidirectional serial console connection +between them (although we don't care much about how this connection +looks like in practice). + +Such a bare-bone operation might require manual intervention directed +by the test host (e.g. the host asking the user to reboot the DUT when +it got stuck). It may also be augmented by additional circuitry that +allows automating these steps. + +These constraints ensure that a test suite can be executed "one-off" +by a developer with minimal hardware investment while also allowing +automated systems to execute the tests without user intervention. + +So these are + +*Requirement 1*: The system must be usable with minimal investment for +manual operation: a host, a DUT and a bidirectional serial connection +between them. + +and + +*Requirement 2*: The system must be able to drive additional controls +to support fully automated operation. + +There will be lots of different types of DUTs, and not all tests +apply to all of them: There's no battery on a desktop board, so no +need to test charging. A test for charging should be automatically +skipped on such a DUT without counting as failure. + +*Requirement 3*: The system must allow to specify DUTs and their +properties, as well as the properties that a test requires to be +meaningful. + +Tests should be self-contained. This means that they should start +from a flash cycle (that a tool like flashrom would optimize away to +a no-op if flash is still in the expected state) and power-on. + +*Requirement 4*: Tests always start with the required firmware image +to write and explicit power-on. + +Tests must be able to state that other tests must have succeeded before +them: A test that exercises boot loader options doesn't fail when the +boot process never gets to the options screen, it just shouldn't run. + +*Requirement 5*: Tests must be able to declare tests they depend on. + +State transitions should be well-defined: The DUT can move from the +power-off state to the power-on state. It can also fail to do so and +remain powered-off. These things need to be spelled out, together +with the consequences that arise from each transition: it's expected +that most transitions would be failures, but that should be noted down. + +And yet, this verbosity shouldn't mean that the tests become +unreadable. Ideally they could be used by a human operator as "regular +text" (with odd characters interspersed) outlining steps to execute +manually, without a test runner. + +*Requirement 6*: It must be simple to state "Initiate action, success +is W, X while failure is Y, Z". + +During the test, the host monitors the serial console and extracts +events from it. Such events could be, for example, "coreboot enters +ramstage". A test can expect events to appear on the stream and fail if +unexpected events appear or no events appear within a given deadline. + +The observed events are reported to the user. + +*Requirement 7*: There's an event parser looking for things that +happen on the DUT. + +The host can send characters to the DUT, for example in a boot loader +or on a terminal provided by an OS that eventually boots. + +*Requirement 8*: Test must be able to run an expect-like language to +drive complex interactions on the serial console. + +Some tests may be better run on the target system (e.g. to parse +out tables in memory, or otherwise inspect boot states). They should +have a minimal set of dependencies so they don't fail because the base +system changed slightly, be portable across architectures and operating +systems. The language should be popular and the implementation robust. + +*Requirement 9*: Implementation language must be popular, robust, +portable and operate with minimal dependencies. + +## Implementation proposal + +The test system will be implemented in Go: it's reasonably popular +(unlike Forth), robust (unlike shell scripts), its statically linked +binaries have minimal dependencies on the host environment (unlike +Python) and it supports multiple architectures and operating systems. + +The build creates a host side test runner (native OS/arch) and a +variable number of DUT test runners (for all supported DUT OS/arch +pairs) that are provided to the DUT on some storage medium (e.g. USB +stick). In a fully automated environment this might require switching +USB ports between host and DUT. + +There's a [liberally licensed implementation of expect][goexpect] that +could be integrated for handling interactive consoles. It supports +parallel execution which is useful to have a listener on the serial +console (when not driven by expect). + +[goexpect]: https://github.com/google/goexpect + +The test runner is called with the names of a set of tests to execute +(individually or as suites that tests can be assigned to) and the +type of DUT to work with. + +From this, it knows how to drive the DUT. The "manual operation" mode +would be just a special type of button driver that asks the user to +conduct some operation and potentially to press Enter. + +Tests are defined in go packages, with per-package set up and tear +down routines. The runner optimizes operations by telling the tear down +routine if the next test will be from the same package (so the amount +of tear down could be reduced). Setup isn't told about the previous +state but should assume a random state. It's only allowed to assume +that the tests it requires to execute earlier have passed. It can +still optimize based on things it measures such as flashrom not writing +unchanged blocks. + +### Flow + +``` + DUT name + Tests to execute +--------------+ + + +---->+Console driver| + | | +--------------+ + | +---->+Button driver | + | | +--------------+ + v v + +-----------+ +----------+ + |Test runner+--------+DUT object+<+ + +-----------+ +----------+ | + | executes |generates | + | v | + | sends+------------+ | control + +<-----+event stream| +----+ + | +------------+ | | + | | | + | +----------+ | + +------------->+Host tests| | + | +----------+ | + | | + | +--------------+ call +------------------+ + +------------->+Target tests +----------+Target test runner| + +--------------+ +------------------+ +``` + diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md index 7c231fc..0df2a1b 100644 --- a/Documentation/technotes/index.md +++ b/Documentation/technotes/index.md @@ -2,3 +2,4 @@ * [Dealing with Untrusted Input in SMM](2017-02-dealing-with-untrusted-input-in-smm.md) * [Rebuilding coreboot image generation](2015-11-rebuilding-coreboot-image-generation.md) +* [firmware test runner](2019-12-firmware-testing.md) -- To view, visit https://review.coreboot.org/c/coreboot/+/37643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb9380050f8ff1aa13ecbb501079e2556e43ca06 Gerrit-Change-Number: 37643 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

6 23

Change in ...coreboot[master]: libpayload: Enable -Wconversion
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32185 Change subject: libpayload: Enable -Wconversion ...................................................................... libpayload: Enable -Wconversion Together with -Werror this ensures that no new conversion related issues can appear. BUG=b:111443775 BRANCH=none TEST=make junit.xml has no failed builds Change-Id: Idc37c50cae90d283bd6ec240dd7de3546485a9a9 Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> --- M payloads/libpayload/Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/32185/1 diff --git a/payloads/libpayload/Makefile.inc b/payloads/libpayload/Makefile.inc index 7787762..ceac4ca 100644 --- a/payloads/libpayload/Makefile.inc +++ b/payloads/libpayload/Makefile.inc @@ -64,7 +64,7 @@ CFLAGS += -ffunction-sections -fdata-sections CFLAGS += -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes CFLAGS += -Wwrite-strings -Wredundant-decls -Wno-trigraphs -CFLAGS += -Wstrict-aliasing -Wshadow -Werror +CFLAGS += -Wstrict-aliasing -Wshadow -Werror -Wconversion $(obj)/libpayload-config.h: $(KCONFIG_AUTOHEADER) cmp $@ $< 2>/dev/null || cp $< $@ -- To view, visit https://review.coreboot.org/c/coreboot/+/32185 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idc37c50cae90d283bd6ec240dd7de3546485a9a9 Gerrit-Change-Number: 32185 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

2 4

Change in ...coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib
by Patrick Georgi (Code Review) 19 Nov '21

19 Nov '21

Hello Julien Viard de Galbert, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/31072 to review the following change. Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib ...................................................................... soc/intel/denverton_ns: Rewrite pmutil using pmclib Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751 Signed-off-by: Julien Viard de Galbert <jviarddegalbert(a)online.net> --- M src/soc/intel/denverton_ns/include/soc/pm.h M src/soc/intel/denverton_ns/pmutil.c M src/soc/intel/denverton_ns/smihandler.c M src/soc/intel/denverton_ns/smm.c 4 files changed, 33 insertions(+), 184 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/31072/1 diff --git a/src/soc/intel/denverton_ns/include/soc/pm.h b/src/soc/intel/denverton_ns/include/soc/pm.h index 09ce158..0aab0e2 100644 --- a/src/soc/intel/denverton_ns/include/soc/pm.h +++ b/src/soc/intel/denverton_ns/include/soc/pm.h @@ -47,18 +47,6 @@ struct chipset_power_state *fill_power_state(void); /* Power Management Utility Functions. */ -uint32_t clear_smi_status(void); -uint16_t clear_pm1_status(void); -uint32_t clear_tco_status(void); -uint32_t clear_gpe_status(void); void clear_pmc_status(void); -void enable_smi(uint32_t mask); -void disable_smi(uint32_t mask); -void enable_pm1(uint16_t events); -void enable_pm1_control(uint32_t mask); -void disable_pm1_control(uint32_t mask); -void enable_gpe(uint32_t mask); -void disable_gpe(uint32_t mask); -void disable_all_gpe(void); #endif /* _DENVERTON_NS_PM_H_ */ diff --git a/src/soc/intel/denverton_ns/pmutil.c b/src/soc/intel/denverton_ns/pmutil.c index ccf0d95..344f8b9 100644 --- a/src/soc/intel/denverton_ns/pmutil.c +++ b/src/soc/intel/denverton_ns/pmutil.c @@ -19,29 +19,12 @@ #include <arch/io.h> #include <console/console.h> +#include <intelblocks/pmclib.h> #include <soc/iomap.h> #include <soc/soc_util.h> #include <soc/pm.h> -static void print_num_status_bits(int num_bits, uint32_t status, - const char *const bit_names[]) -{ - int i; - - if (!status) - return; - - for (i = num_bits - 1; i >= 0; i--) { - if (status & (1 << i)) { - if (bit_names[i]) - printk(BIOS_DEBUG, "%s ", bit_names[i]); - else - printk(BIOS_DEBUG, "BIT%d ", i); - } - } -} - -static uint32_t print_smi_status(uint32_t smi_sts) +const char *const *soc_smi_sts_array(size_t *a) { static const char *const smi_sts_bits[] = { [2] = "BIOS", @@ -67,93 +50,11 @@ [31] = "LEGACY_USB3", }; - if (!smi_sts) - return 0; - - printk(BIOS_DEBUG, "SMI_STS: "); - print_num_status_bits(ARRAY_SIZE(smi_sts_bits), smi_sts, smi_sts_bits); - printk(BIOS_DEBUG, "\n"); - - return smi_sts; + *a = ARRAY_SIZE(smi_sts_bits); + return smi_sts_bits; } -static uint32_t reset_smi_status(void) -{ - uint16_t pmbase = get_pmbase(); - uint32_t smi_sts = inl((uint16_t)(pmbase + SMI_STS)); - outl(smi_sts, (uint16_t)(pmbase + SMI_STS)); - return smi_sts; -} - -uint32_t clear_smi_status(void) { return print_smi_status(reset_smi_status()); } - -void enable_smi(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t smi_en = inl((uint16_t)(pmbase + SMI_EN)); - smi_en |= mask; - outl(smi_en, (uint16_t)(pmbase + SMI_EN)); -} - -void disable_smi(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t smi_en = inl((uint16_t)(pmbase + SMI_EN)); - smi_en &= ~mask; - outl(smi_en, (uint16_t)(pmbase + SMI_EN)); -} - -void enable_pm1_control(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t pm1_cnt = inl((uint16_t)(pmbase + PM1_CNT)); - pm1_cnt |= mask; - outl(pm1_cnt, (uint16_t)(pmbase + PM1_CNT)); -} - -void disable_pm1_control(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t pm1_cnt = inl((uint16_t)(pmbase + PM1_CNT)); - pm1_cnt &= ~mask; - outl(pm1_cnt, (uint16_t)(pmbase + PM1_CNT)); -} - -static uint16_t reset_pm1_status(void) -{ - uint16_t pmbase = get_pmbase(); - uint16_t pm1_sts = inw((uint16_t)(pmbase + PM1_STS)); - outw(pm1_sts, (uint16_t)(pmbase + PM1_STS)); - return pm1_sts; -} - -static uint16_t print_pm1_status(uint16_t pm1_sts) -{ - static const char *const pm1_sts_bits[] = { - [0] = "TMROF", [4] = "BM", [5] = "GBL", - [8] = "PWRBTN", [10] = "RTC", [11] = "PRBTNOR", - [15] = "WAK", - }; - - if (!pm1_sts) - return 0; - - printk(BIOS_SPEW, "PM1_STS: "); - print_num_status_bits(ARRAY_SIZE(pm1_sts_bits), pm1_sts, pm1_sts_bits); - printk(BIOS_SPEW, "\n"); - - return pm1_sts; -} - -uint16_t clear_pm1_status(void) { return print_pm1_status(reset_pm1_status()); } - -void enable_pm1(uint16_t events) -{ - uint16_t pmbase = get_pmbase(); - outw(events, (uint16_t)(pmbase + PM1_EN)); -} - -static uint32_t print_tco_status(uint32_t tco_sts) +const char *const *soc_tco_sts_array(size_t *a) { static const char *const tco_sts_bits[] = { [0] = "NMI2SMI", [1] = "OS_TCO_SMI", @@ -164,17 +65,11 @@ [17] = "SECOND_TO", [20] = "SMLINK_SLV_SMI", }; - if (!tco_sts) - return 0; - - printk(BIOS_DEBUG, "TCO_STS: "); - print_num_status_bits(ARRAY_SIZE(tco_sts_bits), tco_sts, tco_sts_bits); - printk(BIOS_DEBUG, "\n"); - - return tco_sts; + *a = ARRAY_SIZE(tco_sts_bits); + return tco_sts_bits; } -static uint32_t reset_tco_status(void) +uint32_t soc_reset_tco_status(void) { uint16_t tcobase = get_tcobase(); uint32_t tco_sts = inl((uint16_t)(tcobase + TCO1_STS)); @@ -184,35 +79,7 @@ return tco_sts & tco_en; } -uint32_t clear_tco_status(void) { return print_tco_status(reset_tco_status()); } - -void enable_gpe(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t gpe0_en = inl((uint16_t)(pmbase + GPE0_EN(GPE_STD))); - gpe0_en |= mask; - outl(gpe0_en, (uint16_t)(pmbase + GPE0_EN(GPE_STD))); -} - -void disable_gpe(uint32_t mask) -{ - uint16_t pmbase = get_pmbase(); - uint32_t gpe0_en = inl((uint16_t)(pmbase + GPE0_EN(GPE_STD))); - gpe0_en &= ~mask; - outl(gpe0_en, (uint16_t)(pmbase + GPE0_EN(GPE_STD))); -} - -void disable_all_gpe(void) { disable_gpe(~0); } - -static uint32_t reset_gpe_status(void) -{ - uint16_t pmbase = get_pmbase(); - uint32_t gpe_sts = inl((uint16_t)(pmbase + GPE0_STS(GPE_STD))); - outl(gpe_sts, (uint16_t)(pmbase + GPE0_STS(GPE_STD))); - return gpe_sts; -} - -static uint32_t print_gpe_sts(uint32_t gpe_sts) +const char *const *soc_std_gpe_sts_array(size_t *a) { static const char *const gpe_sts_bits[] = { [0] = "GPIO_0", [1] = "GPIO_1", @@ -233,16 +100,8 @@ [30] = "GPIO_30", [31] = "GPIO_31", }; - if (!gpe_sts) - return gpe_sts; - - printk(BIOS_DEBUG, "GPE0a_STS: "); - print_num_status_bits(ARRAY_SIZE(gpe_sts_bits), gpe_sts, gpe_sts_bits); - printk(BIOS_DEBUG, "\n"); - - return gpe_sts; + *a = ARRAY_SIZE(gpe_sts_bits); + return gpe_sts_bits; } -uint32_t clear_gpe_status(void) { return print_gpe_sts(reset_gpe_status()); } - void clear_pmc_status(void) { /* TODO */ } diff --git a/src/soc/intel/denverton_ns/smihandler.c b/src/soc/intel/denverton_ns/smihandler.c index 85f99d7..e1e7bc4 100644 --- a/src/soc/intel/denverton_ns/smihandler.c +++ b/src/soc/intel/denverton_ns/smihandler.c @@ -25,6 +25,7 @@ #include <device/pci_def.h> #include <elog.h> #include <intelblocks/fast_spi.h> +#include <intelblocks/pmclib.h> #include <spi-generic.h> #include <soc/iomap.h> #include <soc/soc_util.h> @@ -52,7 +53,7 @@ return 0; } -void southbridge_smi_set_eos(void) { enable_smi(EOS); } +void southbridge_smi_set_eos(void) { pmc_enable_smi(EOS); } global_nvs_t *smm_get_gnvs(void) { return gnvs; } @@ -98,7 +99,7 @@ uint16_t pmbase = get_pmbase(); /* First, disable further SMIs */ - disable_smi(SLP_SMI_EN); + pmc_disable_smi(SLP_SMI_EN); /* Figure out SLP_TYP */ reg32 = inl((uint16_t)(pmbase + PM1_CNT)); @@ -131,7 +132,7 @@ printk(BIOS_DEBUG, "SMI#: Entering S5 (Soft Power off)\n"); /* Disable all GPE */ - disable_all_gpe(); + pmc_disable_all_gpe(); /* also iterates over all bridges on bus 0 */ busmaster_disable_on_bus(0); @@ -145,7 +146,7 @@ * event again. We need to set BIT13 (SLP_EN) though to make the * sleep happen. */ - enable_pm1_control(SLP_EN); + pmc_enable_pm1_control(SLP_EN); /* Make sure to stop executing code here for S3/S4/S5 */ if (slp_typ > 1) @@ -158,7 +159,7 @@ reg32 = inl((uint16_t)(pmbase + PM1_CNT)); if (reg32 & SCI_EN) { /* The OS is not an ACPI OS, so we set the state to S0 */ - disable_pm1_control(SLP_EN | SLP_TYP); + pmc_disable_pm1_control(SLP_EN | SLP_TYP); } } @@ -237,11 +238,11 @@ printk(BIOS_DEBUG, "P-state control\n"); break; case APM_CNT_ACPI_DISABLE: - disable_pm1_control(SCI_EN); + pmc_disable_pm1_control(SCI_EN); printk(BIOS_DEBUG, "SMI#: ACPI disabled.\n"); break; case APM_CNT_ACPI_ENABLE: - enable_pm1_control(SCI_EN); + pmc_enable_pm1_control(SCI_EN); printk(BIOS_DEBUG, "SMI#: ACPI enabled.\n"); break; case APM_CNT_FINALIZE: @@ -268,23 +269,23 @@ static void southbridge_smi_pm1(void) { - uint16_t pm1_sts = clear_pm1_status(); + uint16_t pm1_sts = pmc_clear_pm1_status(); /* While OSPM is not active, poweroff immediately * on a power button event. */ if (pm1_sts & PWRBTN_STS) { // power button pressed - disable_pm1_control(-1UL); - enable_pm1_control(SLP_EN | (SLP_TYP_S5 << SLP_TYP_SHIFT)); + pmc_disable_pm1_control(-1UL); + pmc_enable_pm1_control(SLP_EN | (SLP_TYP_S5 << SLP_TYP_SHIFT)); } } -static void southbridge_smi_gpe0(void) { clear_gpe_status(); } +static void southbridge_smi_gpe0(void) { pmc_clear_all_gpe_status(); } static void southbridge_smi_tco(void) { - uint32_t tco_sts = clear_tco_status(); + uint32_t tco_sts = pmc_clear_tco_status(); /* Any TCO event? */ if (!tco_sts) @@ -354,7 +355,7 @@ /* We need to clear the SMI status registers, or we won't see what's * happening in the following calls. */ - smi_sts = clear_smi_status(); + smi_sts = pmc_clear_smi_status(); /* Call SMI sub handler for each of the status bits */ for (i = 0; i < ARRAY_SIZE(southbridge_smi); i++) { diff --git a/src/soc/intel/denverton_ns/smm.c b/src/soc/intel/denverton_ns/smm.c index 9d3fa75..65d2499 100644 --- a/src/soc/intel/denverton_ns/smm.c +++ b/src/soc/intel/denverton_ns/smm.c @@ -23,6 +23,7 @@ #include <cpu/x86/smm.h> #include <string.h> +#include <intelblocks/pmclib.h> #include <soc/iomap.h> #include <soc/soc_util.h> #include <soc/pm.h> @@ -48,10 +49,10 @@ } /* Dump and clear status registers */ - clear_smi_status(); - clear_pm1_status(); - clear_tco_status(); - clear_gpe_status(); + pmc_clear_smi_status(); + pmc_clear_pm1_status(); + pmc_clear_tco_status(); + pmc_clear_all_gpe_status(); clear_pmc_status(); } @@ -60,8 +61,8 @@ printk(BIOS_DEBUG, "Enabling SMIs.\n"); /* Configure events Disable pcie wake. */ - enable_pm1(PWRBTN_EN | GBL_EN | PCIEXPWAK_DIS); - disable_gpe(PME_B0_EN); + pmc_enable_pm1(PWRBTN_EN | GBL_EN | PCIEXPWAK_DIS); + pmc_disable_std_gpe(PME_B0_EN); /* Enable SMI generation: * - on APMC writes (io 0xb2) @@ -71,7 +72,7 @@ * - on TCO events * - on microcontroller writes (io 0x62/0x66) */ - enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS); + pmc_enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS); } void smm_setup_structures(void *gnvs, void *tcg, void *smi1) -- To view, visit https://review.coreboot.org/c/coreboot/+/31072 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751 Gerrit-Change-Number: 31072 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Julien Viard de Galbert <coreboot-review-ju(a)vdg.name> Gerrit-MessageType: newchange

5 8

Change in ...coreboot[master]: security/vboot: Do not check for RW partitions if not part of the image
by Werner Zeh (Code Review) 18 Nov '21

18 Nov '21

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31662 Change subject: security/vboot: Do not check for RW partitions if not part of the image ...................................................................... security/vboot: Do not check for RW partitions if not part of the image In the setup where measured boot is used with read-only partition only there is no RW_A or RW_B partition in the flash. In this case it makes no sense to let VBOOT check for these partitions just to fail and then fall back to recovery mode. Instead set the flag VB2_CONTEXT_RECOVERY_MODE right away so that VBOOT starts in recovery mode any time. This kind of bypasses VBOOT logic but is still suitable to have a pure measured boot scheme enabled. In addition it avoids the first two reboots due to missing RW_A and RW_B. Change-Id: I07b8ec97be7db63b7ccddb3f33e0f741bed8acd8 Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com> --- M src/security/vboot/vboot_logic.c 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31662/1 diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 8c3ba80..89934b9 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -324,6 +324,12 @@ die("Initializing measured boot mode failed!"); } + /* Skip checking for RW_A and RW_B if these partitions are not included + in the image. Instead proceed with recovery mode which uses RO + partition only. */ + if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A)) + ctx.flags |= VB2_CONTEXT_RECOVERY_MODE; + if (IS_ENABLED(CONFIG_VBOOT_PHYSICAL_DEV_SWITCH) && get_developer_mode_switch()) ctx.flags |= VB2_CONTEXT_FORCE_DEVELOPER_MODE; -- To view, visit https://review.coreboot.org/c/coreboot/+/31662 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I07b8ec97be7db63b7ccddb3f33e0f741bed8acd8 Gerrit-Change-Number: 31662 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: newchange

5 8

Change in ...coreboot[master]: security/vboot: Added logic to authenticate firmware using VBOOT libr...
by Amol N Sukerkar (Code Review) 15 Nov '21

15 Nov '21

Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32148 Change subject: security/vboot: Added logic to authenticate firmware using VBOOT library version 2.1 ...................................................................... security/vboot: Added logic to authenticate firmware using VBOOT library version 2.1 This feature adds logic to authenticate Coreboot stages using VBOOT libraries version 2.1 Users of this feature will need to enable CONFIG_VBOOT_STAGE_VERIFICATION to enable the corresponding logic in Coreboot to authenticate the VBLOCK version 2.1 and the Coreboot stage hashes. TEST=Create a coreboot.rom image which has keyblock and VBLOCK with VBOOT version 2.1 structures. This is done by enabling CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots to authenticated payload. Change-Id: Ie06d51d180d4c8a872d678d6bd5c67de0efffca2 Signed-off-by: Sukerkar, Amol N <amol.n.sukerkar(a)intel.com> --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_loader.c A src/security/vboot/vboot_logic_ex.c 5 files changed, 475 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32148/1 diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ca25423..e7bd3f9 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -2,6 +2,7 @@ ## ## Copyright (C) 2014 The ChromiumOS Authors. All rights reserved. ## Copyright (C) 2018 Siemens AG +## Copyright (C) 2019 Intel Corp. All rights reserved. ## ## This program is free software; you can redistribute it and/or modify ## it under the terms of the GNU General Public License as published by @@ -53,6 +54,12 @@ help Have two update partitions beside the RO partition. +config VBOOT_STAGE_VERIFICATION + bool "Verify stages with vboot 2.1" + default n + help + Enables vboot verification for each stage + config VBOOT_VBNV_CMOS bool default n @@ -323,18 +330,52 @@ config VBOOT_ROOT_KEY string "Root key (public)" default "$(VBOOT_SOURCE)/tests/devkeys/root_key.vbpubk" + depends on !VBOOT_STAGE_VERIFICATION config VBOOT_RECOVERY_KEY string "Recovery key (public)" default "$(VBOOT_SOURCE)/tests/devkeys/recovery_key.vbpubk" + depends on !VBOOT_STAGE_VERIFICATION config VBOOT_FIRMWARE_PRIVKEY string "Firmware key (private)" default "$(VBOOT_SOURCE)/tests/devkeys/firmware_data_key.vbprivk" + depends on !VBOOT_STAGE_VERIFICATION config VBOOT_KERNEL_KEY string "Kernel subkey (public)" default "$(VBOOT_SOURCE)/tests/devkeys/kernel_subkey.vbpubk" + depends on !VBOOT_STAGE_VERIFICATION + +config VBOOT_2_1_ROOT_KEY + string "Root Key version 2.1 (public)" + default "$(VBOOT_SOURCE)/tests/devkeys/root_key.vbpubk2" + depends on VBOOT_STAGE_VERIFICATION + +config VBOOT_2_1_PRIVATE_KEY + string "Root Key version 2.1 (private)" + default "$(VBOOT_SOURCE)/tests/devkeys/root_key.vbprik2" + depends on VBOOT_STAGE_VERIFICATION + +config VBOOT_2_1_FW_PRE_PUBLIC_KEY + string "FW Preamble Key version 2.1 (public)" + default "$(VBOOT_SOURCE)/tests/devkeys/fw_pre.vbpubk2" + depends on VBOOT_STAGE_VERIFICATION + +config VBOOT_2_1_FW_PRE_PRIVATE_KEY + string "FW PREAMBLE Key version 2.1 (private)" + default "$(VBOOT_SOURCE)/tests/devkeys/fw_pre.vbprik2" + depends on VBOOT_STAGE_VERIFICATION + +config VBOOT_2_1_RAMSTAGE_HASH_KEY + string "Coreboot RAMStage Stage Hashing Key(private)" + default "$(VBOOT_SOURCE)/tests/devkeys/rhash.vbprik2" + depends on VBOOT_STAGE_VERIFICATION + +config VBOOT_2_1_PAYLOAD_HASH_KEY + string "Coreboot PAYLOAD Stage Hashing Key(private)" + default "$(VBOOT_SOURCE)/tests/devkeys/phash.vbprik2" + depends on VBOOT_STAGE_VERIFICATION config VBOOT_KEYBLOCK string "Keyblock to use for the RW regions" diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 6d2096d..a65b066 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -3,6 +3,7 @@ ## ## Copyright (C) 2014 The ChromiumOS Authors. All rights reserved. ## Copyright (C) 2018 Siemens AG +## Copyright (C) 2019 Intel Corp. All rights reserved. ## ## This program is free software; you can redistribute it and/or modify ## it under the terms of the GNU General Public License as published by @@ -35,16 +36,27 @@ bootblock-y += vbnv.c verstage-y += vbnv.c romstage-y += vbnv.c +postcar-$(CONFIG_VBOOT_STAGE_VERIFICATION) += vbnv.c ramstage-y += vbnv.c bootblock-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c verstage-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c romstage-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c + +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION),y) +postcar-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c +endif + ramstage-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c bootblock-$(CONFIG_VBOOT_VBNV_CMOS_BACKUP_TO_FLASH) += vbnv_flash.c verstage-$(CONFIG_VBOOT_VBNV_CMOS_BACKUP_TO_FLASH) += vbnv_flash.c romstage-$(CONFIG_VBOOT_VBNV_CMOS_BACKUP_TO_FLASH) += vbnv_flash.c + +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION),y) +postcar-$(CONFIG_VBOOT_VBNV_CMOS_BACKUP_TO_FLASH) += vbnv_flash.c +endif + ramstage-$(CONFIG_VBOOT_VBNV_CMOS_BACKUP_TO_FLASH) += vbnv_flash.c bootblock-$(CONFIG_VBOOT_VBNV_EC) += vbnv_ec.c @@ -77,12 +89,26 @@ endif bootblock-y += common.c + +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION),y) +verstage-y += vboot_logic_ex.c +else verstage-y += vboot_logic.c +endif +postcar-$(CONFIG_VBOOT_STAGE_VERIFICATION) += vboot_logic_ex.c +ramstage-$(CONFIG_VBOOT_STAGE_VERIFICATION) += vboot_logic_ex.c + verstage-y += common.c verstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += verstage.c ifeq (${CONFIG_VBOOT_MOCK_SECDATA},y) verstage-y += secdata_mock.c romstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_mock.c + +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION),y) +postcar-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_mock.c +ramstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_mock.c +endif + else verstage-y += secdata_tpm.c romstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_tpm.c @@ -101,7 +127,11 @@ # call with $1 = stage name to create rules for building the library # for the stage and adding it to the stage's set of object files. define vboot-for-stage +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION),y) +VB2_LIB_$(1) = $(obj)/external/vboot_reference-$(1)/vboot_fw21.a +else VB2_LIB_$(1) = $(obj)/external/vboot_reference-$(1)/vboot_fw20.a +endif VBOOT_CFLAGS_$(1) += $$(call vboot-fixup-includes,$$(CPPFLAGS_$(1))) VBOOT_CFLAGS_$(1) += $$(CFLAGS_$(1)) VBOOT_CFLAGS_$(1) += $$(call vboot-fixup-includes,$$($(1)-c-ccopts)) @@ -116,7 +146,8 @@ $(MAKE) -C $(VBOOT_SOURCE) \ BUILD=$$(abspath $$(dir $$(VB2_LIB_$(1)))) \ V=$(V) \ - fwlib20 + $(if $(filter y,$(CONFIG_VBOOT_STAGE_VERIFICATION)), \ + fwlib21, fwlib20) $(1)-srcs += $$(VB2_LIB_$(1)) @@ -129,6 +160,11 @@ $(eval $(call vboot-for-stage,ramstage)) $(eval $(call vboot-for-stage,postcar)) +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION),y) +postcar-srcs += $(VB2_LIB) +ramstage-srcs += $(VB2_LIB) +endif + ifeq ($(CONFIG_VBOOT_SEPARATE_VERSTAGE),y) $(eval $(call vboot-for-stage,verstage)) @@ -236,12 +272,21 @@ $(obj)/gbb.region: $(obj)/gbb.stub @printf " SETUP GBB\n" cp $< $@.tmp +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION), y) + $(FUTILITY) gbb_utility -s \ + --hwid="$(CONFIG_GBB_HWID)" \ + --rootkey="$(CONFIG_VBOOT_2_1_ROOT_KEY)" \ + --flags=$(GBB_FLAGS) \ + $@.tmp +else $(FUTILITY) gbb_utility -s \ --hwid="$(CONFIG_GBB_HWID)" \ --rootkey="$(CONFIG_VBOOT_ROOT_KEY)" \ --recoverykey="$(CONFIG_VBOOT_RECOVERY_KEY)" \ --flags=$(GBB_FLAGS) \ $@.tmp +endif # CONFIG_VBOOT_STAGE_VERIFICATION + ifneq ($(CONFIG_GBB_BMPFV_FILE),) $(FUTILITY) gbb_utility -s \ --bmpfv="$(CONFIG_GBB_BMPFV_FILE)" \ @@ -288,6 +333,47 @@ mv $@.tmp2 $@ rm -f $@.tmp $@.tmp.size +ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION), y) +# Extract RAMStage and PAYLOAD and hash them +# Since we are booting from FW_MAIN_A, RAMStage and payload are +# extracted from FW_MAIN_A. +$(obj)/ramstage.hash: $(obj)/coreboot.rom + @printf " CREATE RAMSTAGE HASH\n" + $(CBFSTOOL) $< print -r FW_MAIN_A > $<.tmp + python $(VBOOT_SOURCE)/compute_hash.py -i $<.tmp -o $@.tmp -c $< -f ramstage + $(FUTILITY) --vb21 sign \ + --type rwsig \ + --prikey "$(CONFIG_VBOOT_2_1_RAMSTAGE_HASH_KEY)" \ + $@.tmp $@ + rm -f $<.tmp $@.tmp + +$(obj)/payload.hash: $(obj)/coreboot.rom + @printf " CREATE PAYLOAD HASH\n" + $(CBFSTOOL) $< print -r FW_MAIN_A > $<.tmp + python $(VBOOT_SOURCE)/compute_hash.py -i $<.tmp -o $@.tmp -c $< -f payload + $(FUTILITY) --vb21 sign \ + --type rwsig \ + --prikey "$(CONFIG_VBOOT_2_1_PAYLOAD_HASH_KEY)" \ + $@.tmp $@ + rm -f $<.tmp $@.tmp + +$(obj)/firmware.kb21: $(FUTILITY) + $(FUTILITY) vbutil_keyblock \ + --pack21 $@ \ + --signprivate "$(CONFIG_VBOOT_2_1_PRIVATE_KEY)" \ + --datapubkey "$(CONFIG_VBOOT_2_1_FW_PRE_PUBLIC_KEY)" \ + --flags $(CONFIG_VBOOT_KEYBLOCK_PREAMBLE_FLAGS) + +$(obj)/VBLOCK_%.bin: $(obj)/FW_MAIN_%.bin $(FUTILITY) \ + $(obj)/ramstage.hash $(obj)/payload.hash $(obj)/firmware.kb21 + $(FUTILITY) vbutil_firmware \ + --vblock21 $@ \ + --keyblock "$(top)/$(obj)/firmware.kb21" \ + --signprepriv "$(CONFIG_VBOOT_2_1_FW_PRE_PRIVATE_KEY)" \ + --hashdir "$(top)/$(obj)" \ + --version $(CONFIG_VBOOT_KEYBLOCK_VERSION) \ + --flags $(CONFIG_VBOOT_KEYBLOCK_PREAMBLE_FLAGS) +else $(obj)/VBLOCK_%.bin: $(obj)/FW_MAIN_%.bin $(FUTILITY) $(FUTILITY) vbutil_firmware \ --vblock $@ \ @@ -297,6 +383,7 @@ --fv $< \ --kernelkey "$(CONFIG_VBOOT_KERNEL_KEY)" \ --flags $(CONFIG_VBOOT_KEYBLOCK_PREAMBLE_FLAGS) +endif # CONFIG_VBOOT_STAGE_VERIFICATION ifeq ($(CONFIG_VBOOT_SLOTS_RW_AB),y) files_added:: $(obj)/VBLOCK_A.bin $(obj)/VBLOCK_B.bin @@ -314,3 +401,4 @@ endif endif # CONFIG_VBOOT + diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index 1f6d4ee..c8980b6 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2013 Google, Inc. + * Copyright (C) 2019 Intel Corp. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -170,10 +171,12 @@ * Therefore, the vboot results would not be initialized so don't * automatically add results when cbmem comes online. */ -#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) +#if !IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE) && \ + !IS_ENABLED(CONFIG_VBOOT_STAGE_VERIFICATION) static void vb2_fill_handoff_cbmem(int unused) { vboot_fill_handoff(); } ROMSTAGE_CBMEM_INIT_HOOK(vb2_fill_handoff_cbmem) #endif + diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 0640ebd..e09a314 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright 2015 Google, Inc. + * Copyright 2019 Intel Corp. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -60,6 +61,16 @@ return 0; } +/* This is the helper function that decides when the stage verification + * code should be called. */ +static int stage_verification_should_run(void) +{ + if (IS_ENABLED(CONFIG_VBOOT_STAGE_VERIFICATION)) + return ENV_POSTCAR | ENV_RAMSTAGE; + + return 0; +} + static int vboot_executed CAR_GLOBAL; int vboot_logic_executed(void) @@ -86,9 +97,11 @@ static void vboot_prepare(void) { - if (verification_should_run()) { + if (verification_should_run() || + stage_verification_should_run()) { /* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */ verstage_main(); + car_set_var(vboot_executed, 1); vboot_save_recovery_reason_vbnv(); } else if (verstage_should_load()) { @@ -154,3 +167,4 @@ .prepare = vboot_prepare, .locate = vboot_locate, }; + diff --git a/src/security/vboot/vboot_logic_ex.c b/src/security/vboot/vboot_logic_ex.c new file mode 100644 index 0000000..7a735a9 --- /dev/null +++ b/src/security/vboot/vboot_logic_ex.c @@ -0,0 +1,326 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <security/tpm/antirollback.h> +#include <arch/exception.h> +#include <assert.h> +#include <bootmode.h> +#include <console/console.h> +#include <console/vtxprintf.h> +#include <delay.h> +#include <string.h> +#include <timestamp.h> +#include <vb2_api.h> +#include <security/vboot/misc.h> +#include <security/vboot/vbnv.h> + +#include <cbfs.h> +#include <cbmem.h> +#include <rmodule.h> + +/* For individual stage verification design, only + * RW Region A is relevant. So, force CBFS to bot + * from RW Region A */ +static void reset_fw_slot(struct vb2_context *ctx) +{ + ctx->flags &= ~VB2_CONTEXT_FW_SLOT_B; +} + +/* Decide which RW region we are booting from */ +static int is_slot_a(struct vb2_context *ctx) +{ + return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); +} + +/* exports */ + +void vb2ex_printf (const char *func, const char *fmt, ...) +{ + va_list args; + + if (func) + printk(BIOS_INFO, "VB2:%s() ", func); + + va_start(args, fmt); + vprintk(BIOS_INFO, fmt, args); + va_end(args); + + return; +} + +int vb2ex_tpm_clear_owner(struct vb2_context *ctx) +{ + uint32_t rv; + printk(BIOS_INFO, "Clearing TPM owner\n"); + rv = tpm_clear_and_reenable(); + if (rv) + return VB2_ERROR_EX_TPM_CLEAR_OWNER; + return VB2_SUCCESS; +} + +int vb2ex_read_resource(struct vb2_context *ctx, + enum vb2_resource_index index, + uint32_t offset, + void *buf, + uint32_t size) +{ + struct region_device rdev; + const char *name; + + switch (index) { + case VB2_RES_GBB: + name = "GBB"; + break; + case VB2_RES_FW_VBLOCK: + if (is_slot_a(ctx)) + name = "VBLOCK_A"; + else + name = "VBLOCK_B"; + break; + default: + return VB2_ERROR_EX_READ_RESOURCE_INDEX; + } + + if (vboot_named_region_device(name, &rdev)) + return VB2_ERROR_EX_READ_RESOURCE_SIZE; + + if (rdev_readat(&rdev, buf, offset, size) != size) + return VB2_ERROR_EX_READ_RESOURCE_SIZE; + + return VB2_SUCCESS; +} + +/* No-op stubs that can be overridden by SoCs with hardware crypto support. */ +__attribute__((weak)) +int vb2ex_hwcrypto_digest_init(enum vb2_hash_algorithm hash_alg, + uint32_t data_size) +{ + return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; +} + +__attribute__((weak)) +int vb2ex_hwcrypto_digest_extend(const uint8_t *buf, uint32_t size) +{ + BUG(); /* Should never get called if init() returned an error. */ + return VB2_ERROR_UNKNOWN; +} + +__attribute__((weak)) +int vb2ex_hwcrypto_digest_finalize(uint8_t *digest, uint32_t digest_size) +{ + BUG(); /* Should never get called if init() returned an error. */ + return VB2_ERROR_UNKNOWN; +} + +static int locate_firmware(struct vb2_context *ctx, + struct region_device *fw_main) +{ + const char *name = "FW_MAIN_A"; + + if (is_slot_a(ctx)) + name = "FW_MAIN_A"; + else + name = "FW_MAIN_B"; + + return vboot_named_region_device(name, fw_main); +} + +/** + * Save non-volatile and/or secure data if needed. + */ +static void save_if_needed(struct vb2_context *ctx) +{ + if (ctx->flags & VB2_CONTEXT_NVDATA_CHANGED) { + printk(BIOS_INFO, "Saving nvdata\n"); + save_vbnv(ctx->nvdata); + ctx->flags &= ~VB2_CONTEXT_NVDATA_CHANGED; + } + if (ctx->flags & VB2_CONTEXT_SECDATA_CHANGED) { + printk(BIOS_INFO, "Saving secdata\n"); + antirollback_write_space_firmware(ctx); + ctx->flags &= ~VB2_CONTEXT_SECDATA_CHANGED; + } +} + +/* VB2 context initialization helper function */ +static void init_ctx(struct vb2_context *ctx) +{ + int rv; + + /* Set up context and work buffer */ + vboot_init_work_context(ctx); + + /* Initialize and read nvdata from non-volatile storage. */ + vbnv_init(ctx->nvdata); + + /* This function is required as it reads secdata (use mock secdat) + * If secdata is not valid, call to vb2api_fw_phase1 will fail. So, + * even if this is not required in the current context, call it. */ + antirollback_read_space_firmware(ctx); + + /* Do early init (set up secdata and NVRAM, load GBB) */ + printk(BIOS_INFO, "Phase 1\n"); + rv = vb2api_fw_phase1(ctx); + if (rv) { + printk(BIOS_INFO, "Reboot requested (%x)\n", rv); + save_if_needed(ctx); + vboot_reboot(); + } + + /* Determine which firmware slot to boot (based on NVRAM) */ + printk(BIOS_INFO, "Phase 2\n"); + rv = vb2api_fw_phase2(ctx); + reset_fw_slot(ctx); + if (rv) { + printk(BIOS_INFO, "Reboot requested (%x)\n", rv); + save_if_needed(ctx); + vboot_reboot(); + } +} + +/* hash verification helper function */ +static int verify_hash(struct vb2_context *ctx, void *map, + const struct vb2_id *id) +{ + int rv; + uint32_t size = 0; + + printk(BIOS_INFO, "Phase 4\n"); + + /* init hash */ + rv = vb21api_init_hash(ctx, id, &size); + if (rv) + return rv; + + /* extend hash over the body */ + rv = vb2api_extend_hash(ctx,(uint8_t *)map, size); + if (rv) + return rv; + + /* check hash */ + rv = vb21api_check_hash(ctx); + if (rv) + return rv; + + return rv; +} + +/* initialize the VB2 context in VERStage, so + * we know which RW partition to boot the rest + * of the stages from */ +static void init_ctx_verstage(void) +{ + struct vb2_context ctx; + struct region_device fw_main; + int rv; + + init_ctx(&ctx); + + rv = locate_firmware(&ctx, &fw_main); + if (rv) + die("Failed to read FMAP to locate firmware"); + + printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(&ctx) ? 'A' : 'B'); + + vboot_set_selected_region(region_device_region(&fw_main)); +} + +/* Veify the stage to be executed */ +static void verify_stage(void) +{ + struct vb2_context ctx; + struct region_device fw_main; + int rv; + const struct region_device *fh = NULL; + size_t fsize = 0; + void *map = NULL; + struct cbfsf file; + const struct vb2_id* id; + + /* For each stage to be verified, extract map and + * hashing algo */ + if (ENV_POSTCAR) { + printk(BIOS_INFO, "Verify ramstage\n"); + id = vb2_hash_id(VB2_HASH_SHA256); + struct prog stage = PROG_INIT(PROG_RAMSTAGE, + CONFIG_CBFS_PREFIX "/ramstage"); + + /* load stage */ + if (cbfs_boot_locate(&file, prog_name(&stage), NULL)) + die("failed to load stage"); + + cbfs_file_data(prog_rdev(&stage), &file); + fh = &stage.rdev; + + fsize = region_device_sz(fh); + map = rdev_mmap(fh, 0, fsize); + if (!map) printk(BIOS_INFO, "ERROR: Mapping failed\n"); + } else if (ENV_RAMSTAGE) { + printk(BIOS_INFO, "Verify payload\n"); + id = vb2_hash_id(VB2_HASH_SHA512); + struct prog stage = PROG_INIT(PROG_PAYLOAD, + CONFIG_CBFS_PREFIX "/payload"); + + /* load stage */ + if (cbfs_boot_locate(&file, prog_name(&stage), NULL)) + die("failed to load stage"); + + cbfs_file_data(prog_rdev(&stage), &file); + fh = &stage.rdev; + + fsize = region_device_sz(fh); + map = rdev_mmap(fh, 0, fsize); + if (!map) printk(BIOS_INFO, "ERROR: Mapping failed\n"); + } else + die("Impossible"); + + //get_stage_attr(&map, &id); + /* initialize the vb context and read the NV data */ + init_ctx(&ctx); + + rv = locate_firmware(&ctx, &fw_main); + if (rv) + die("Failed to read FMAP to locate firmware"); + + /* Try the slot (verify its keyblock and preamble) */ + printk(BIOS_INFO, "Phase 3\n"); + rv = vb21api_fw_phase3(&ctx); + if (rv) { + printk(BIOS_INFO, "Reboot requested (%x)\n", rv); + save_if_needed(&ctx); + vboot_reboot(); + } + + /* verify the hash */ + rv = verify_hash(&ctx, map, id); + if (rv) { + printk(BIOS_ERR, "ERROR:0x%x ", rv); + die("Stage Verification Failed"); + } + + rdev_munmap(fh, map); + + printk(BIOS_INFO, "stage verified successfully, proceed...\n"); +} + +/* Main Entry Point for Stage Verification */ +void verstage_main(void) +{ + if (ENV_VERSTAGE) + init_ctx_verstage(); + else if (ENV_POSTCAR || ENV_RAMSTAGE) + verify_stage(); +} + -- To view, visit https://review.coreboot.org/c/coreboot/+/32148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie06d51d180d4c8a872d678d6bd5c67de0efffca2 Gerrit-Change-Number: 32148 Gerrit-PatchSet: 1 Gerrit-Owner: Amol N Sukerkar <amol.n.sukerkar(a)intel.com> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: src/lib: Disable display init skip when VBOOT Stage Verification is e...
by Amol N Sukerkar (Code Review) 15 Nov '21

15 Nov '21

Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32149 Change subject: src/lib: Disable display init skip when VBOOT Stage Verification is enabled ...................................................................... src/lib: Disable display init skip when VBOOT Stage Verification is enabled When VBOOT is enabled, by default it native display init is skipped and custom display init mechanism is utilized. VBOOT Stage Verification utilizes native display init. This change implements that feature. TEST=Create a coreboot.rom image by enabling CONFIG_VBOOT and CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots to authenticated payload and graphics is displayed via HDMI and Display Port. Change-Id: I65a96ec74f7b494d0c16814d84067e004ceebe70 Signed-off-by: Sukerkar, Amol N <amol.n.sukerkar(a)intel.com> --- M src/lib/bootmode.c 1 file changed, 7 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/32149/1 diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index e402536..2cf9bd0 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. + * Copyright (C) 2019 Intel Corporation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -34,8 +35,12 @@ int display_init_required(void) { - /* For Chrome OS always honor vboot_handoff_skip_display_init(). */ - if (CONFIG(CHROMEOS)) + /* For Chrome OS always honor vboot_handoff_skip_display_init(). + * A special case is when CONFIG_VBOOT_STAGE_VERIFICATION is + * enabled. In that case, vboot_handoff_skip_display_init() + * is overridden native display init mechanism. */ + if (IS_ENABLED(CONFIG_CHROMEOS) && + !IS_ENABLED(CONFIG_VBOOT_STAGE_VERIFICATION)) return !vboot_handoff_skip_display_init(); /* By default always initialize display. */ -- To view, visit https://review.coreboot.org/c/coreboot/+/32149 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I65a96ec74f7b494d0c16814d84067e004ceebe70 Gerrit-Change-Number: 32149 Gerrit-PatchSet: 1 Gerrit-Owner: Amol N Sukerkar <amol.n.sukerkar(a)intel.com> Gerrit-MessageType: newchange

2 1

Change in ...coreboot[master]: src/include/cbfs.h: Added prototype for function to verify a stage af...
by Amol N Sukerkar (Code Review) 15 Nov '21

15 Nov '21

Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32150 Change subject: src/include/cbfs.h: Added prototype for function to verify a stage after it has been loaded into DRAM ...................................................................... src/include/cbfs.h: Added prototype for function to verify a stage after it has been loaded into DRAM This support enables a user to implement a stage verification mechanism AFTER the stage has been loaded into DRAM. This feature is currently used by VBOOT_STAGE_VERIFICATION TEST=Create a coreboot.rom image which has keyblock and VBLOCK with VBOOT version 2.1 structures. This is done by enabling CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots to authenticated payload. Change-Id: I8702f00186db568316e04ffb87fd1439a27bdb6e Signed-off-by: Sukerkar, Amol N <amol.n.sukerkar(a)intel.com> --- M src/include/cbfs.h 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/32150/1 diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 85e25b3..d24fe58 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright 2015 Google Inc. + * Copyright 2019 Intel Corp. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -90,4 +91,9 @@ int (*locate)(struct cbfs_props *props); }; +/* This function can be used to implement any secure boot mechanism to + * verify the stage AFTER it is loaded into DRAM */ +void verify_stage_if_required(const struct region_device *rdev); + #endif + -- To view, visit https://review.coreboot.org/c/coreboot/+/32150 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8702f00186db568316e04ffb87fd1439a27bdb6e Gerrit-Change-Number: 32150 Gerrit-PatchSet: 1 Gerrit-Owner: Amol N Sukerkar <amol.n.sukerkar(a)intel.com> Gerrit-MessageType: newchange

2 1

Change in ...coreboot[master]: src/lib: Implemented function calls to verify a stage after it has be...
by Amol N Sukerkar (Code Review) 15 Nov '21

15 Nov '21

Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32151 Change subject: src/lib: Implemented function calls to verify a stage after it has been loaded into DRAM ...................................................................... src/lib: Implemented function calls to verify a stage after it has been loaded into DRAM This support enables a user to implement a stage verification mechanism AFTER the stage has been loaded into DRAM. This feature is currently used by VBOOT_STAGE_VERIFICATION TEST=Create a coreboot.rom image which has keyblock and VBLOCK with VBOOT version 2.1 structures. This is done by enabling CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots to authenticated payload. Change-Id: I649f511bc5375448dd7625b57a680135395d1062 Signed-off-by: Sukerkar, Amol N <amol.n.sukerkar(a)intel.com> --- M src/lib/Kconfig M src/lib/cbfs.c M src/lib/selfboot.c 3 files changed, 59 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/32151/1 diff --git a/src/lib/Kconfig b/src/lib/Kconfig index 2f10c1c..8111f8d 100644 --- a/src/lib/Kconfig +++ b/src/lib/Kconfig @@ -30,6 +30,13 @@ Selected by features that require to parse and manipulate a flattened devicetree in ramstage. +config VERIFY_ONLY_PAYLOAD_IN_RAMSTAGE + bool "Verify only payload in ramstage" + default n + help + Selected by features that require verified boot but when only payload + is verified in ramstage. + if RAMSTAGE_LIBHWBASE config HWBASE_DYNAMIC_MMIO diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 728674f..9da0fae 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -3,6 +3,7 @@ * * Copyright (C) 2011 secunet Security Networks AG * Copyright 2015 Google Inc. + * Copyright 2019 Intel Corp. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -36,6 +37,28 @@ #define DEBUG(x...) #endif +/* This is marked as weak so some verification mechanism can + * use it to verify after loading into DRAM. Primarily + * overriden by VBOOT mechanism. + */ +void __weak verify_stage_if_required(const struct region_device *rdev) +{ + /* no op */ +} + +/* This function checks if a certain stage/binary meets the criteria + * to be verified AFTER it is loaded into DRAM + */ +static int cbfs_verification_meets_criteria(void) +{ + /* if this is true, it means we are verifying nothing here + * that is loaded in RAMSTAGE */ + if (IS_ENABLED(CONFIG_VERIFY_ONLY_PAYLOAD_IN_RAMSTAGE)) + return !ENV_RAMSTAGE; + else + return 0; +} + int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type) { struct region_device rdev; @@ -109,6 +132,11 @@ return 0; if (rdev_readat(rdev, buffer, offset, in_size) != in_size) return 0; + + /* If the stage/binary loaded in DRAM requires verification + * proceed if it meets the required criteria */ + if (cbfs_verification_meets_criteria()) + verify_stage_if_required(rdev); return in_size; case CBFS_COMPRESS_LZ4: @@ -127,6 +155,12 @@ timestamp_add_now(TS_START_ULZ4F); out_size = ulz4fn(compr_start, in_size, buffer, buffer_size); timestamp_add_now(TS_END_ULZ4F); + + /* If the stage/binary loaded in DRAM requires verification + * proceed if it meets the required criteria */ + if (cbfs_verification_meets_criteria()) + verify_stage_if_required(rdev); + return out_size; case CBFS_COMPRESS_LZMA: @@ -149,6 +183,11 @@ rdev_munmap(rdev, map); + /* If the stage/binary loaded in DRAM requires verification + * proceed if it meets the required criteria */ + if (cbfs_verification_meets_criteria()) + verify_stage_if_required(rdev); + return out_size; default: diff --git a/src/lib/selfboot.c b/src/lib/selfboot.c index 9aa4741..3d438e8 100644 --- a/src/lib/selfboot.c +++ b/src/lib/selfboot.c @@ -4,6 +4,7 @@ * Copyright (C) 2003 Eric W. Biederman <ebiederm(a)xmission.com> * Copyright (C) 2009 Ron Minnich <rminnich(a)gmail.com> * Copyright (C) 2016 George Trudeau <george.trudeau(a)usherbrooke.ca> + * Copyright (C) 2019 Intel Corp. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -32,6 +33,15 @@ /* The type syntax for C is essentially unparsable. -- Rob Pike */ typedef int (*checker_t)(struct cbfs_payload_segment *cbfssegs, void *args); +/* This is marked as weak so some verification mechanism can + * use it to verify after loading into DRAM. Primarily + * overriden by VBOOT mechanism. + */ +void __weak verify_stage_if_required(const struct region_device *rdev) +{ + /* no op */ +} + /* Decode a serialized cbfs payload segment * from memory into native endianness. */ @@ -269,6 +279,9 @@ rdev_munmap(prog_rdev(payload), data); + /* verify payload using a secure boot mechanism if required */ + verify_stage_if_required(prog_rdev(payload)); + /* Pass cbtables to payload if architecture desires it. */ prog_set_entry(payload, (void *)entry, cbmem_find(CBMEM_ID_CBTABLE)); -- To view, visit https://review.coreboot.org/c/coreboot/+/32151 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I649f511bc5375448dd7625b57a680135395d1062 Gerrit-Change-Number: 32151 Gerrit-PatchSet: 1 Gerrit-Owner: Amol N Sukerkar <amol.n.sukerkar(a)intel.com> Gerrit-MessageType: newchange

3 2

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2020