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@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(); } }
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41120
to look at the new patch set (#2).
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_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/2
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41120
to look at the new patch set (#5).
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c 13 files changed, 216 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/5
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41120
to look at the new patch set (#6).
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c 13 files changed, 217 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h:
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... PS8, Line 14: hash above, which may be shorter than the compile-time size of struct vb2_hash! */ What's the reason for doing this as opposed to honoring the fixed length of the object itself?
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... PS8, Line 25: /* Must *not* appear anywhere else in coreboot code (other than the anchor). */ Wouldn't the act of verifying the magic value include the immediate value? Update: looks like we're not looking for this string in the code.
Maybe we should note that more explicitly. And if it's not supposed to be referenced anywhere in the code why provide a #define in a header file for people to obtain? For building purposes? vs runtime?
https://review.coreboot.org/c/coreboot/+/41120/8/src/include/metadata_hash.h File src/include/metadata_hash.h:
https://review.coreboot.org/c/coreboot/+/41120/8/src/include/metadata_hash.h... PS8, Line 12: struct vb2_hash *metadata_hash_get(void); could you please provide some commentary as to what this function would be used for and when?
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@36 PS8, Line 36: if (cbd != vboot_get_cbfs_boot_device()) { I'm confused by this check. This error is only true when !NO_CBFS_MCACHE as well as !recovery. I see the Kconfig dependencies, but the code would be clearer if !NO_CBFS_MCACHE as added here as well. Separately, cbfs_boot_lookup() has force_ro parameter so it's not clear to me this is a valid check w/o checking force_ro here too.
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@471 PS8, Line 471: die("RO CBFS initialization error: %d", err); I'm still having a hard time grok'ing why it's ok to fill up the mcache and not fail for Chrome OS. It makes it really hard to deal w/ checking integrity of the files' contents when the metadata is only sitting on boot media which means you'll have to re-walk the whole cbfs for a missing file lookup in mcache.
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/fmap.c@37 PS8, Line 37: ENV_BOOTBLOCK ENV_INITIAL_STAGE?
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/metadata_hash.c File src/lib/metadata_hash.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/metadata_hash.c@10 PS8, Line 10: struct metadata_hash_anchor metadata_hash_anchor = { why is this global? and not const?
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41120
to look at the new patch set (#9).
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c 13 files changed, 219 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/9
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41120
to look at the new patch set (#10).
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c M util/cbfstool/cbfs.h 14 files changed, 238 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/10
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h:
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... PS8, Line 14: hash above, which may be shorter than the compile-time size of struct vb2_hash! */
What's the reason for doing this as opposed to honoring the fixed length of the object itself?
The length of struct vb2_hash (and the value of VB2_MAX_DIGEST_SIZE) may change based on whether any `#define VB2_SUPPORT_<algo>` macros are defined when including <vb2_sha.h> (see https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs...), and I expect it might change in the future if support for different algorithms with longer hashes is added, so I am careful to not rely on it in any serialized structure. I'll expand this comment a bit to make that clearer.
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... PS8, Line 25: /* Must *not* appear anywhere else in coreboot code (other than the anchor). */
Wouldn't the act of verifying the magic value include the immediate value? Update: looks like we're […]
I do use it in src/lib/metadata_hash.c to define the anchor, it's just important that it not be used anywhere else in coreboot. It's used in cbfstool to find the anchor (that's why I'm defining it here in commonlib so that both can use the same definition).
I expanded the comment a bit and put DO_NOT_USE in the name to make it more visible.
https://review.coreboot.org/c/coreboot/+/41120/8/src/include/metadata_hash.h File src/include/metadata_hash.h:
https://review.coreboot.org/c/coreboot/+/41120/8/src/include/metadata_hash.h... PS8, Line 12: struct vb2_hash *metadata_hash_get(void);
could you please provide some commentary as to what this function would be used for and when?
Done
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@36 PS8, Line 36: if (cbd != vboot_get_cbfs_boot_device()) {
I'm confused by this check. This error is only true when !NO_CBFS_MCACHE as well as !recovery. […]
TOCTOU_SAFETY implies !NO_CBFS_MCACHE via Kconfig so I'm not checking it explicitly again.
This check is basically a "are we looking at the RO CBFS" check -- I know it looks a bit weird but that was the best way I found to do it (I can't just look at force_ro, because even for !force_ro we may still be looking at the RO CBFS in early stages).
When we are looking at the RO CBFS and the cache is full, then we cannot continue because we may not be in the bootblock anymore and other stages do not have access to the metadata hash anchor. I basically decided to force TOCTOU_SAFETY to depend on !NO_CBFS_MCACHE (and !NO_FMAP_CACHE) so I could avoid the problem of having to carry the metadata anchor itself forward into other stages. I think that's okay because the RO mcache either overflows right away or not at all -- you don't have that problem that it may overflow later with a post-ship update, so I'm basically saying people who need TOCTOU safety have to ensure their RO cache is big enough from the get go. (For RW we won't have that problem because the RW metadata hash will be part of vb2_context and available in all RW stages. See also the help text for the TOCTOU_SAFETY option in Kconfig.)
Actually, I think I can also just check this right when the mcache is built (where you put your other comment), maybe that's cleaner?
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@471 PS8, Line 471: die("RO CBFS initialization error: %d", err);
I'm still having a hard time grok'ing why it's ok to fill up the mcache and not fail for Chrome OS. […]
See above -- if TOCTOU_SAFETY is enabled, then an overflow of the RO mcache is basically a fatal error. Moving that check down here now.
For configurations without TOCTOU_SAFETY (and we don't intend to use it in Chrome OS), the metadata only needs to be checked once. cbfs_mcache_build() still checks the whole metadata even if the cache was full halfway through, and it prioritizes CB_CBFS_HASH_MISMATCH over CB_CBFS_CACHE_FULL -- so if we get here and we get CACHE_FULL, we know the metadata for the whole CBFS is valid. Since we don't care about TOCTOU attacks we can later during the same boot just re-read it and trust it if we're trying to look up a file beyond what we fit in our cache. (Note that you don't exactly rewalk the whole CBFS in that case, just up to the file you're trying to find, since you're not checking the metadata hash again at that time -- and cbfs_walk() is smart enough to abort early and to avoid reading extra attribute data in that case.)
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/fmap.c@37 PS8, Line 37: ENV_BOOTBLOCK
ENV_INITIAL_STAGE?
Right, thanks.
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/metadata_hash.c File src/lib/metadata_hash.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/metadata_hash.c@10 PS8, Line 10: struct metadata_hash_anchor metadata_hash_anchor = {
why is this global? and not const?
The const goes back to the discussion in https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... -- the argument to cbfs_walk() cannot be const, so if I made this struct const I'd have to cast it away at some point.
With static I think I was worried that might interact weirdly with the __attribute__((section)) somehow, but when trying it out now it seem to work fine. So okay, added.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@36 PS8, Line 36: if (cbd != vboot_get_cbfs_boot_device()) {
TOCTOU_SAFETY implies !NO_CBFS_MCACHE via Kconfig so I'm not checking it explicitly again. […]
I'm not completely following, but I'll take a look at the updated CL if it's clearer.
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@471 PS8, Line 471: die("RO CBFS initialization error: %d", err);
See above -- if TOCTOU_SAFETY is enabled, then an overflow of the RO mcache is basically a fatal err […]
I think that's where I wasn't understanding. You had assembled almost everything required to be TOCTOU safe, and I was assuming we'd actually employ that for Chrome OS since we're so close. I absolutely think we should strive for that instead of leaving the opportunity open for attacks. i.e. we're tightening up the threat model.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/41120/10/src/include/cbfs_glue.h File src/include/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/41120/10/src/include/cbfs_glue.h@10 PS10, Line 10: #define CBFS_ENABLE_HASHING (CONFIG(CBFS_VERIFICATION) && \ Could you please add a comment covering the conditions and reasoning? i.e. why only when ENV_INITIAL_STAGE == true?
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41120
to look at the new patch set (#14).
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c M util/cbfstool/cbfs.h 14 files changed, 248 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/14
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41120/10/src/include/cbfs_glue.h File src/include/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/41120/10/src/include/cbfs_glue.h@10 PS10, Line 10: #define CBFS_ENABLE_HASHING (CONFIG(CBFS_VERIFICATION) && \
Could you please add a comment covering the conditions and reasoning? i.e. […]
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 14: Code-Review+2
Patrick Georgi has uploaded a new patch set (#16) to the change originally created by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c M util/cbfstool/cbfs.h 14 files changed, 248 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41120/16
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
cbfs: Add verification for RO CBFS metadata 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 metadata 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 "metadata hash anchor" structure that is embedded in the bootblock code and marked by a unique magic number. This anchor contains both the CBFS metadata 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 metadata 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@chromium.org Change-Id: I8930434de55eb938b042fdada9aa90218c0b5a34 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41120 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- A src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h M src/commonlib/include/commonlib/cbmem_id.h M src/include/cbfs.h M src/include/cbfs_glue.h A src/include/metadata_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/metadata_hash.c M src/lib/program.ld M src/security/Kconfig M src/security/vboot/vboot_loader.c M util/cbfstool/cbfs.h 14 files changed, 248 insertions(+), 27 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h b/src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h new file mode 100644 index 0000000..d5e54b5 --- /dev/null +++ b/src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-only */ + +#ifndef _COMMONLIB_BSD_METADATA_HASH_H_ +#define _COMMONLIB_BSD_METADATA_HASH_H_ + +#include <stdint.h> +#include <vb2_sha.h> + +/* This structure is embedded somewhere in the (uncompressed) bootblock. */ +struct metadata_hash_anchor { + uint8_t magic[8]; + struct vb2_hash cbfs_hash; + /* NOTE: This is just reserving space. sizeof(struct vb2_hash) may change between + configurations/versions and cannot be relied upon, so the FMAP hash must be placed + right after the actual data for the particular CBFS hash algorithm used ends. */ + 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 *metadata_hash_anchor_fmap_hash(struct metadata_hash_anchor *anchor) +{ + return anchor->cbfs_hash.raw + vb2_digest_size(anchor->cbfs_hash.algo); +} + +/* + * Do not use this constant anywhere else in coreboot code to ensure the bit pattern really only + * appears once in the CBFS image. The only coreboot file allowed to use this is + * src/lib/metadata_anchor.c to define the actual anchor data structure. It is defined here so + * that it can be shared with cbfstool (which may use it freely). + */ +#define DO_NOT_USE_METADATA_HASH_ANCHOR_MAGIC_DO_NOT_USE "\xadMdtHsh\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 ab7cf63..6e24545 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_MDATA_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 8d4c220..cad01c6 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. * @@ -74,4 +75,13 @@ */ const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro);
+/* + * Builds the mcache (if |cbd->mcache| is set) and verifies |metadata_hash| (if + * it is not NULL). If CB_CBFS_CACHE_FULL is returned, the mcache is incomplete + * but still valid and the metadata 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 *metadata_hash); + #endif diff --git a/src/include/cbfs_glue.h b/src/include/cbfs_glue.h index ebfbc2e..ffca83e 100644 --- a/src/include/cbfs_glue.h +++ b/src/include/cbfs_glue.h @@ -5,8 +5,19 @@
#include <commonlib/region.h> #include <console/console.h> +#include <rules.h>
-#define CBFS_ENABLE_HASHING 0 +/* + * This flag prevents linking hashing functions into stages where they're not required. We don't + * need them at all if verification is disabled. If verification is enabled without TOCTOU + * safety, we only need to verify the metadata hash in the initial stage and can assume it stays + * valid in later stages. If TOCTOU safety is required, we may need them in every stage to + * reverify metadata that had to be reloaded from flash (e.g. because it didn't fit the mcache). + * Note that this only concerns metadata hashing -- file access functions may still link hashing + * routines independently for file data hashing. + */ +#define CBFS_ENABLE_HASHING (CONFIG(CBFS_VERIFICATION) && \ + (CONFIG(TOCTOU_SAFETY) || ENV_INITIAL_STAGE))
#define ERROR(...) printk(BIOS_ERR, "CBFS ERROR: " __VA_ARGS__) #define LOG(...) printk(BIOS_ERR, "CBFS: " __VA_ARGS__) diff --git a/src/include/metadata_hash.h b/src/include/metadata_hash.h new file mode 100644 index 0000000..2d3b8a8 --- /dev/null +++ b/src/include/metadata_hash.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef _METADATA_HASH_H_ +#define _METADATA_HASH_H_ + +#include <commonlib/bsd/metadata_hash.h> + +/* Verify the an FMAP data structure with the FMAP hash that is stored together with the CBFS + metadata hash in the bootblock's metadata hash anchor (when CBFS verification is enabled). */ +vb2_error_t metadata_hash_verify_fmap(const void *fmap_base, size_t fmap_size); + +#if CONFIG(CBFS_VERIFICATION) +/* Get the (RO) CBFS metadata hash for this CBFS image, which forms the root of trust for CBFS + verification. This function is only available in the bootblock. */ +struct vb2_hash *metadata_hash_get(void); +#else +static inline struct vb2_hash *metadata_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..3499345 --- /dev/null +++ b/src/lib/Kconfig.cbfs_verification @@ -0,0 +1,57 @@ +# 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 + depends on !VBOOT_STARTS_BEFORE_BOOTBLOCK # this is gonna get tricky... + select VBOOT_LIB + 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 + 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. + +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 6cff03d..9e601eb 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -47,6 +47,7 @@ bootblock-y += cbfs.c bootblock-$(CONFIG_GENERIC_GPIO_LIB) += gpio.c bootblock-y += libgcc.c +bootblock-$(CONFIG_CBFS_VERIFICATION) += metadata_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 beab74e..5df1d8b 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -10,6 +10,7 @@ #include <console/console.h> #include <fmap.h> #include <lib.h> +#include <metadata_hash.h> #include <security/tpm/tspi/crtm.h> #include <security/vboot/vboot_common.h> #include <stdlib.h> @@ -29,8 +30,21 @@ if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM) 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 *metadata_hash = NULL; + if (CONFIG(TOCTOU_SAFETY)) { + if (ENV_SMM) /* Cannot provide TOCTOU safety for SMM */ + dead_code(); + /* We can only reach this for the RW CBFS -- an mcache + overflow in the 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. */ + } + err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, + metadata_hash); + }
if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro && err == CB_CBFS_NOT_FOUND) { @@ -405,6 +419,26 @@ } }
+cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd, + struct vb2_hash *metadata_hash) +{ + /* If we have an mcache, mcache_build() will also check mdata hash. */ + if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM && cbd->mcache_size > 0) + return cbfs_mcache_build(&cbd->rdev, cbd->mcache, + cbd->mcache_size, metadata_hash); + + /* No mcache and no verification means we have nothing special to do. */ + if (!CONFIG(CBFS_VERIFICATION) || !metadata_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, metadata_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; @@ -426,15 +460,18 @@ return &ro;
if (fmap_locate_area_as_rdev("COREBOOT", &ro.rdev)) - return NULL; + die("Cannot locate primary CBFS");
cbfs_boot_device_find_mcache(&ro, CBMEM_ID_CBFS_RO_MCACHE);
- if (ENV_INITIAL_STAGE && !CONFIG(NO_CBFS_MCACHE)) { - 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_INITIAL_STAGE) { + cb_err_t err = cbfs_init_boot_device(&ro, metadata_hash_get()); + if (err == CB_CBFS_HASH_MISMATCH) + die("RO CBFS metadata hash verification failure"); + else if (CONFIG(TOCTOU_SAFETY) && err == CB_CBFS_CACHE_FULL) + die("RO mcache overflow breaks TOCTOU safety!\n"); + 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 377123a..2abe138 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -4,6 +4,7 @@ #include <cbmem.h> #include <console/console.h> #include <fmap.h> +#include <metadata_hash.h> #include <stddef.h> #include <string.h> #include <symbols.h> @@ -27,9 +28,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_INITIAL_STAGE || done) + return 0; /* Only need to check hash in first stage. */ + + if (metadata_hash_verify_fmap(fmap, FMAP_SIZE) != VB2_SUCCESS) + return -1; + + done = true; + return 0; }
static void report(const struct fmap *fmap) @@ -63,10 +75,12 @@ if (!(ENV_INITIAL_STAGE)) { /* NOTE: This assumes that the first stage 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. */ @@ -80,7 +94,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);
@@ -111,8 +125,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/metadata_hash.c b/src/lib/metadata_hash.c new file mode 100644 index 0000000..f296cf5 --- /dev/null +++ b/src/lib/metadata_hash.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <assert.h> +#include <cbmem.h> +#include <metadata_hash.h> +#include <symbols.h> + +__attribute__((used, section(".metadata_hash_anchor"))) +static struct metadata_hash_anchor metadata_hash_anchor = { + /* This is the only place in all of coreboot where we actually need to use this. */ + .magic = DO_NOT_USE_METADATA_HASH_ANCHOR_MAGIC_DO_NOT_USE, + .cbfs_hash = { .algo = CONFIG_CBFS_HASH_ALGO } +}; + +struct vb2_hash *metadata_hash_get(void) +{ + return &metadata_hash_anchor.cbfs_hash; +} + +vb2_error_t metadata_hash_verify_fmap(const void *fmap_buffer, size_t fmap_size) +{ + struct vb2_hash hash = { .algo = metadata_hash_anchor.cbfs_hash.algo }; + memcpy(hash.raw, metadata_hash_anchor_fmap_hash(&metadata_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 3b6aa2e..94ba409 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -18,6 +18,7 @@ #if !ENV_X86 && (ENV_DECOMPRESSOR || ENV_BOOTBLOCK && !CONFIG(COMPRESS_BOOTBLOCK)) KEEP(*(.id)); #endif + KEEP(*(.metadata_hash_anchor)); *(.text); *(.text.*);
diff --git a/src/security/Kconfig b/src/security/Kconfig index 54d38fb..abbd0b8 100644 --- a/src/security/Kconfig +++ b/src/security/Kconfig @@ -1,5 +1,9 @@ # SPDX-License-Identifier: GPL-2.0-only
+# 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 9c6e56e..56a0664 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(); } }
diff --git a/util/cbfstool/cbfs.h b/util/cbfstool/cbfs.h index 9b4d7ae..7f07751 100644 --- a/util/cbfstool/cbfs.h +++ b/util/cbfstool/cbfs.h @@ -11,6 +11,11 @@
#define CBFS_HEADPTR_ADDR_X86 0xFFFFFFFC
+/* cbfstool is allowed to use this constant freely since it's not part of the + CBFS image, so make an alias for the name that's a little less aggressive. */ +#define METADATA_HASH_ANCHOR_MAGIC \ + DO_NOT_USE_METADATA_HASH_ANCHOR_MAGIC_DO_NOT_USE + struct typedesc_t { uint32_t type; const char *name;
Attention is currently required from: Julius Werner. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 19:
(1 comment)
File src/lib/Kconfig.cbfs_verification:
PS19: I think, that this file can be reworked to match current state. CBFS_HASH_* are unused AFAIK.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 19:
(1 comment)
File src/lib/Kconfig.cbfs_verification:
PS19:
I think, that this file can be reworked to match current state. […]
No, it's used and (last time I tested, at least) it works as intended (see src/lib/metadata_hash.c).
Attention is currently required from: Julius Werner. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 19:
(1 comment)
File src/lib/Kconfig.cbfs_verification:
PS19:
No, it's used and (last time I tested, at least) it works as intended (see src/lib/metadata_hash.c).
Oh, my apologies. I didn't notice it in my grep output.
Regarding the comment above, I was referring to the file as a whole. So, #TODO comments, and WIP messages. Values CBFS_VERIFICATION and TOCTOU_SAFETY are being used in the code and at least the first one works, so TODO/WIP messages should not be there anymore.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 19:
(1 comment)
File src/lib/Kconfig.cbfs_verification:
PS19:
Oh, my apologies. I didn't notice it in my grep output. […]
Hmmm... yeah, I think I was waiting on removing the old APIs first before enabling this because only then it actually works everywhere, but I guess I did that now. So okay, fair enough, should be ready for use. CB:59982
-
Aaron Durbin (Code Review) -
Jakub Czapiga (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)