Change in coreboot[master]: cbfstool: Support CONFIG_CBFS_VERIFICATION and master hash anchor
Hello Patrick Georgi, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41121
to review the following change.
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and master hash anchor ......................................................................
cbfstool: Support CONFIG_CBFS_VERIFICATION and master hash anchor
This patch adds support for the new CONFIG_CBFS_VERIFICATION feature to cbfstool. When CBFS verification is enabled, cbfstool must automatically add a hash attribute to every CBFS file it adds (with a handful of exceptions like bootblock and "header" pseudofiles that are never read by coreboot code itself). It must also automatically update the master hash that is embedded in the bootblock code. It will automatically find the master hash by scanning the bootblock for its magic number and use its presence to auto-detect whether CBFS verification is enabled for an image (and which hash algorithm to use).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I61a84add8654f60c683ef213b844a11b145a5cb7 --- M util/cbfstool/Makefile.inc A util/cbfstool/cbfs_glue.h M util/cbfstool/cbfs_sections.h M util/cbfstool/cbfstool.c 4 files changed, 243 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41121/1
diff --git a/util/cbfstool/Makefile.inc b/util/cbfstool/Makefile.inc index 000ceb2..e98b52e 100644 --- a/util/cbfstool/Makefile.inc +++ b/util/cbfstool/Makefile.inc @@ -23,10 +23,8 @@ cbfsobj += xdr.o cbfsobj += partitioned_file.o # COMMONLIB -cbfsobj += cbfs.o +cbfsobj += cbfs_private.o cbfsobj += fsp_relocate.o -cbfsobj += mem_pool.o -cbfsobj += region.o # FMAP cbfsobj += fmap.o cbfsobj += kv_pair.o @@ -72,10 +70,6 @@ ifitobj += cbfs-mkstage.o ifitobj += cbfs-mkpayload.o ifitobj += rmodule.o -# COMMONLIB -ifitobj += cbfs.o -ifitobj += mem_pool.o -ifitobj += region.o # FMAP ifitobj += fmap.o ifitobj += kv_pair.o @@ -101,6 +95,7 @@ TOOLCPPFLAGS ?= -D_DEFAULT_SOURCE # memccpy() from string.h TOOLCPPFLAGS += -D_BSD_SOURCE -D_SVID_SOURCE # _DEFAULT_SOURCE for older glibc TOOLCPPFLAGS += -D_XOPEN_SOURCE=700 # strdup() from string.h +TOOLCPPFLAGS += -D_GNU_SOURCE # memmem() from string.h TOOLCPPFLAGS += -I$(top)/util/cbfstool/flashmap TOOLCPPFLAGS += -I$(top)/util/cbfstool TOOLCPPFLAGS += -I$(objutil)/cbfstool @@ -207,9 +202,7 @@ # Tolerate lzma sdk warnings $(objutil)/cbfstool/LzmaEnc.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual # Tolerate commonlib warnings -$(objutil)/cbfstool/region.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual -$(objutil)/cbfstool/cbfs.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual -$(objutil)/cbfstool/mem_pool.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual +$(objutil)/cbfstool/cbfs_private.o: TOOLCFLAGS += -Wno-sign-compare # Tolerate lz4 warnings $(objutil)/cbfstool/lz4.o: TOOLCFLAGS += -Wno-missing-prototypes $(objutil)/cbfstool/lz4_wrapper.o: TOOLCFLAGS += -Wno-attributes diff --git a/util/cbfstool/cbfs_glue.h b/util/cbfstool/cbfs_glue.h new file mode 100644 index 0000000..11786be --- /dev/null +++ b/util/cbfstool/cbfs_glue.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _CBFS_GLUE_H_ +#define _CBFS_GLUE_H_ + +#include "cbfs_image.h" + +#define CBFS_ENABLE_HASHING 1 + +typedef const struct cbfs_image *cbfs_dev_t; + +static inline ssize_t cbfs_dev_read(cbfs_dev_t dev, void *buffer, size_t offset, size_t size) +{ + if (buffer_size(&dev->buffer) < offset || + buffer_size(&dev->buffer) - offset < size) + return -1; + + memcpy(buffer, buffer_get(&dev->buffer) + offset, size); + return size; +} + +static inline size_t cbfs_dev_size(cbfs_dev_t dev) +{ + return buffer_size(&dev->buffer); +} + +#endif /* _CBFS_GLUE_H_ */ diff --git a/util/cbfstool/cbfs_sections.h b/util/cbfstool/cbfs_sections.h index 3526f8d..d444cce 100644 --- a/util/cbfstool/cbfs_sections.h +++ b/util/cbfstool/cbfs_sections.h @@ -22,6 +22,7 @@
#define SECTION_NAME_FMAP "FMAP" #define SECTION_NAME_PRIMARY_CBFS "COREBOOT" +#define SECTION_NAME_BOOTBLOCK "BOOTBLOCK"
#define SECTION_ANNOTATION_CBFS "CBFS"
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 6015603..bc07b6f 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -31,13 +31,13 @@ #include "cbfs_sections.h" #include "elfparsing.h" #include "partitioned_file.h" +#include <commonlib/bsd/cbfs_private.h> +#include <commonlib/bsd/master_hash.h> #include <commonlib/fsp.h> #include <commonlib/endian.h> #include <commonlib/helpers.h> #include <vboot_host.h>
-#define SECTION_WITH_FIT_TABLE "BOOTBLOCK" - struct command { const char *name; const char *optstring; @@ -104,6 +104,144 @@ .u64val = -1, };
+struct mh_cache { + const char *region; + size_t offset; + struct vb2_hash cbfs_hash; +}; + +static struct mh_cache *get_mh_cache(void) +{ + static struct mh_cache mhc; + + if (mhc.region) + return &mhc; + + const struct fmap *fmap = partitioned_file_get_fmap(param.image_file); + if (!fmap) + goto no_master_hash; + + size_t offset, size; + struct buffer buffer; + if (fmap_find_area(fmap, SECTION_NAME_BOOTBLOCK)) { + if (!partitioned_file_read_region(&buffer, param.image_file, + SECTION_NAME_BOOTBLOCK)) + goto no_master_hash; + mhc.region = SECTION_NAME_BOOTBLOCK; + offset = 0; + size = buffer.size; + } else { + struct cbfs_image cbfs; + struct cbfs_file *bootblock; + if (!partitioned_file_read_region(&buffer, param.image_file, + SECTION_NAME_PRIMARY_CBFS)) + goto no_master_hash; + mhc.region = SECTION_NAME_PRIMARY_CBFS; + if (cbfs_image_from_buffer(&cbfs, &buffer, param.headeroffset)) + goto no_master_hash; + bootblock = cbfs_get_entry(&cbfs, "bootblock"); + if (!bootblock || ntohl(bootblock->type) != CBFS_TYPE_BOOTBLOCK) + goto no_master_hash; + offset = (void *)bootblock + ntohl(bootblock->offset) - + buffer_get(&cbfs.buffer); + size = ntohl(bootblock->len); + } + + struct master_hash_anchor *anchor = memmem(buffer_get(&buffer) + offset, + size, MASTER_HASH_ANCHOR_MAGIC, sizeof(anchor->magic)); + if (anchor) { + if (!vb2_digest_size(anchor->cbfs_hash.algo)) { + ERROR("Unknown CBFS master hash type: %d\n", + anchor->cbfs_hash.algo); + goto no_master_hash; + } + mhc.cbfs_hash = anchor->cbfs_hash; + mhc.offset = (void *)anchor - buffer_get(&buffer); + return &mhc; + } + +no_master_hash: + mhc.cbfs_hash.algo = VB2_HASH_INVALID; + mhc.region = (void *)-1; + return &mhc; +} + +static void update_and_info(const char *name, void *dst, void *src, size_t size) +{ + if (!memcmp(dst, src, size)) + return; + char *src_str = bintohex(src, size); + char *dst_str = bintohex(dst, size); + INFO("Updating %s from %s to %s\n", name, dst_str, src_str); + memcpy(dst, src, size); + free(src_str); + free(dst_str); +} + +static int update_anchor(struct mh_cache *mhc, uint8_t *fmap_hash) +{ + struct buffer buffer; + if (!partitioned_file_read_region(&buffer, param.image_file, + mhc->region)) + return -1; + struct master_hash_anchor *anchor = buffer_get(&buffer) + mhc->offset; + if (memcmp(anchor->magic, MASTER_HASH_ANCHOR_MAGIC, + sizeof(anchor->magic)) || + anchor->cbfs_hash.algo != mhc->cbfs_hash.algo) { + ERROR("Master hash anchor no longer where it used to be!\n"); + return -1; + } + update_and_info("CBFS master hash", anchor->cbfs_hash.raw, + mhc->cbfs_hash.raw, vb2_digest_size(anchor->cbfs_hash.algo)); + if (fmap_hash) { + update_and_info("FMAP hash", + master_hash_anchor_fmap_hash(anchor), fmap_hash, + vb2_digest_size(anchor->cbfs_hash.algo)); + } + if (!partitioned_file_write_region(param.image_file, &buffer)) + return -1; + return 0; + +} + +static int maybe_update_master_hash(struct cbfs_image *cbfs) +{ + if (strcmp(param.region_name, SECTION_NAME_PRIMARY_CBFS)) + return 0; /* Master hash only embedded in primary CBFS. */ + + struct mh_cache *mhc = get_mh_cache(); + if (mhc->cbfs_hash.algo == VB2_HASH_INVALID) + return 0; + + cb_err_t err = cbfs_walk(cbfs, NULL, NULL, &mhc->cbfs_hash, + CBFS_WALK_WRITEBACK_HASH); + if (err != CB_CBFS_NOT_FOUND) { + ERROR("Unexpected cbfs_walk() error %d\n", err); + return -1; + } + + return update_anchor(mhc, NULL); +} + +static int maybe_update_fmap_hash(void) +{ + if (strcmp(param.region_name, SECTION_NAME_BOOTBLOCK) && + strcmp(param.region_name, SECTION_NAME_FMAP) && + param.type != CBFS_TYPE_BOOTBLOCK) + return 0; /* FMAP and bootblock didn't change. */ + + struct mh_cache *mhc = get_mh_cache(); + if (mhc->cbfs_hash.algo == VB2_HASH_INVALID) + return 0; + + uint8_t fmap_hash[VB2_MAX_DIGEST_SIZE]; + const struct fmap *fmap = partitioned_file_get_fmap(param.image_file); + if (!fmap || vb2_digest_buffer((const void *)fmap, fmap_size(fmap), + mhc->cbfs_hash.algo, fmap_hash, sizeof(fmap_hash))) + return -1; + return update_anchor(mhc, fmap_hash); +} + static bool region_is_flashmap(const char *region) { return partitioned_file_region_check_magic(param.image_file, region, @@ -253,13 +391,21 @@
header = cbfs_create_file_header(CBFS_TYPE_RAW, buffer.size, name); + + enum vb2_hash_algorithm algo = get_mh_cache()->cbfs_hash.algo; + if (algo != VB2_HASH_INVALID) + if (cbfs_add_file_hash(header, &buffer, algo)) { + ERROR("couldn't add hash for '%s'\n", name); + goto done; + } + if (cbfs_add_entry(&image, &buffer, offset, header, 0) != 0) { ERROR("Failed to add %llu into ROM image as '%s'.\n", (long long unsigned)u64val, name); goto done; }
- ret = 0; + ret = maybe_update_master_hash(&image);
done: free(header); @@ -366,6 +512,7 @@ h->offset = htonl(offset); h->architecture = htonl(CBFS_ARCHITECTURE_UNKNOWN);
+ /* Never add a hash attribute to the master header. */ header = cbfs_create_file_header(CBFS_TYPE_CBFSHEADER, buffer_size(&buffer), name); if (cbfs_add_entry(&image, &buffer, 0, header, 0) != 0) { @@ -396,7 +543,7 @@ return 1; }
- ret = 0; + ret = maybe_update_master_hash(&image);
done: free(header); @@ -497,18 +644,32 @@
if (convert && convert(&buffer, &offset, header) != 0) { ERROR("Failed to parse file '%s'.\n", filename); - buffer_delete(&buffer); - return 1; + goto error; }
- if (param.hash != VB2_HASH_INVALID) - if (cbfs_add_file_hash(header, &buffer, param.hash) == -1) { - ERROR("couldn't add hash for '%s'\n", name); - free(header); - buffer_delete(&buffer); - return 1; + /* Bootblock and CBFS header should never have file hashes. When adding + the bootblock it is important that we *don't* look up the master hash + yet (before it is added) or we'll cache an outdated result. */ + if (type != CBFS_TYPE_BOOTBLOCK && type != CBFS_TYPE_CBFSHEADER) { + enum vb2_hash_algorithm mh_algo = get_mh_cache()->cbfs_hash.algo; + if (mh_algo != VB2_HASH_INVALID && param.hash != mh_algo) { + if (param.hash == VB2_HASH_INVALID) { + param.hash = mh_algo; + } else { + ERROR("Cannot specify hash %s that's different from master hash algorithm %s\n", + vb2_get_hash_algorithm_name(param.hash), + vb2_get_hash_algorithm_name(mh_algo)); + goto error; + } }
+ if (param.hash != VB2_HASH_INVALID && + cbfs_add_file_hash(header, &buffer, param.hash) == -1) { + ERROR("couldn't add hash for '%s'\n", name); + goto error; + } + } + if (param.autogen_attr) { /* Add position attribute if assigned */ if (param.baseaddress_assigned || param.stage_xip) { @@ -573,14 +734,18 @@ -offset); if (cbfs_add_entry(&image, &buffer, offset, header, len_align) != 0) { ERROR("Failed to add '%s' into ROM image.\n", filename); - free(header); - buffer_delete(&buffer); - return 1; + goto error; }
free(header); buffer_delete(&buffer); - return 0; + + return maybe_update_master_hash(&image) || maybe_update_fmap_hash(); + +error: + free(header); + buffer_delete(&buffer); + return 1; }
static int cbfstool_convert_raw(struct buffer *buffer, @@ -925,7 +1090,7 @@ return 1; }
- return 0; + return maybe_update_master_hash(&image); }
static int cbfs_create(void) @@ -1096,6 +1261,33 @@ cbfs_print_directory(&image); }
+ if (verbose) { + struct mh_cache *mhc = get_mh_cache(); + if (mhc->cbfs_hash.algo == VB2_HASH_INVALID) + return 0; + + struct vb2_hash real_hash = { .algo = mhc->cbfs_hash.algo }; + cb_err_t err = cbfs_walk(&image, NULL, NULL, &real_hash, + CBFS_WALK_WRITEBACK_HASH); + if (err != CB_CBFS_NOT_FOUND) { + ERROR("Unexpected cbfs_walk() error %d\n", err); + return 1; + } + char *hash_str = bintohex(real_hash.raw, + vb2_digest_size(real_hash.algo)); + printf("[MASTER HASH]\t%s:%s", + vb2_get_hash_algorithm_name(real_hash.algo), hash_str); + if (!strcmp(param.region_name, SECTION_NAME_PRIMARY_CBFS)) { + if (!memcmp(mhc->cbfs_hash.raw, real_hash.raw, + vb2_digest_size(real_hash.algo))) + printf(":valid"); + else + printf(":invalid"); + } + printf("\n"); + free(hash_str); + } + return 0; }
@@ -1194,7 +1386,8 @@ memcpy(param.image_region->data + offset, new_content.data, new_content.size); buffer_delete(&new_content); - return 0; + + return maybe_update_fmap_hash(); }
static int cbfs_read(void) @@ -1454,7 +1647,7 @@ "Find a place for a file of that size\n" " layout [-w] " "List mutable (or, with -w, readable) image regions\n" - " print [-r image,regions] " + " print [-r image,regions] [-k] " "Show the contents of the ROM\n" " extract [-r image,regions] [-m ARCH] -n NAME -f FILE [-U] " "Extracts a file from ROM\n"
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and master hash anchor ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/1/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/1/util/cbfstool/cbfstool.c@65... PS1, Line 659: ERROR("Cannot specify hash %s that's different from master hash algorithm %s\n", line over 96 characters
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/+/41121
to look at the new patch set (#2).
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor
This patch adds support for the new CONFIG_CBFS_VERIFICATION feature to cbfstool. When CBFS verification is enabled, cbfstool must automatically add a hash attribute to every CBFS file it adds (with a handful of exceptions like bootblock and "header" pseudofiles that are never read by coreboot code itself). It must also automatically update the metadata hash that is embedded in the bootblock code. It will automatically find the metadata hash by scanning the bootblock for its magic number and use its presence to auto-detect whether CBFS verification is enabled for an image (and which hash algorithm to use).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I61a84add8654f60c683ef213b844a11b145a5cb7 --- M util/cbfstool/Makefile.inc A util/cbfstool/cbfs_glue.h M util/cbfstool/cbfs_sections.h M util/cbfstool/cbfstool.c 4 files changed, 243 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41121/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/2/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/2/util/cbfstool/cbfstool.c@65... PS2, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/3/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/3/util/cbfstool/cbfstool.c@65... PS3, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/4/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/4/util/cbfstool/cbfstool.c@65... PS4, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/5/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/5/util/cbfstool/cbfstool.c@65... PS5, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
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/+/41121
to look at the new patch set (#6).
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor
This patch adds support for the new CONFIG_CBFS_VERIFICATION feature to cbfstool. When CBFS verification is enabled, cbfstool must automatically add a hash attribute to every CBFS file it adds (with a handful of exceptions like bootblock and "header" pseudofiles that are never read by coreboot code itself). It must also automatically update the metadata hash that is embedded in the bootblock code. It will automatically find the metadata hash by scanning the bootblock for its magic number and use its presence to auto-detect whether CBFS verification is enabled for an image (and which hash algorithm to use).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I61a84add8654f60c683ef213b844a11b145a5cb7 --- M util/cbfstool/Makefile.inc A util/cbfstool/cbfs_glue.h M util/cbfstool/cbfs_sections.h M util/cbfstool/cbfstool.c 4 files changed, 244 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41121/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/6/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/6/util/cbfstool/cbfstool.c@65... PS6, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/7/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/7/util/cbfstool/cbfstool.c@65... PS7, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@65... PS8, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@91 PS8, Line 91: struct mh_cache { I don't see comments, if any, in this patch. It's hard to follow intended semantics.
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@14... PS8, Line 149: (void *)-1 Why are we updating a pointer to this value? -1 is also not a valid pointer size.
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@17... PS8, Line 175: ERROR("Metadata hash anchor no longer where it used to be!\n"); Is that possible? If so, we should discuss how that could happen in the commentary of the code.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/9/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/9/util/cbfstool/cbfstool.c@65... PS9, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/10/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/10/util/cbfstool/cbfstool.c@6... PS10, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/11/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/11/util/cbfstool/cbfstool.c@6... PS11, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/12/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/12/util/cbfstool/cbfstool.c@6... PS12, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/13/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/13/util/cbfstool/cbfstool.c@6... PS13, Line 655: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
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/+/41121
to look at the new patch set (#14).
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor
This patch adds support for the new CONFIG_CBFS_VERIFICATION feature to cbfstool. When CBFS verification is enabled, cbfstool must automatically add a hash attribute to every CBFS file it adds (with a handful of exceptions like bootblock and "header" pseudofiles that are never read by coreboot code itself). It must also automatically update the metadata hash that is embedded in the bootblock code. It will automatically find the metadata hash by scanning the bootblock for its magic number and use its presence to auto-detect whether CBFS verification is enabled for an image (and which hash algorithm to use).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I61a84add8654f60c683ef213b844a11b145a5cb7 --- M util/cbfstool/Makefile.inc A util/cbfstool/cbfs_glue.h M util/cbfstool/cbfs_sections.h M util/cbfstool/cbfstool.c 4 files changed, 262 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41121/14
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@91 PS8, Line 91: struct mh_cache {
I don't see comments, if any, in this patch. It's hard to follow intended semantics.
Added a couple of comments.
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@14... PS8, Line 149: (void *)-1
Why are we updating a pointer to this value? -1 is also not a valid pointer size.
I was just using the .region member to check whether the structure has been initialized yet, so I needed to store a non-NULL bogus value here. I instead added an explicit `initialized` struct member now to make it more readable.
https://review.coreboot.org/c/coreboot/+/41121/8/util/cbfstool/cbfstool.c@17... PS8, Line 175: ERROR("Metadata hash anchor no longer where it used to be!\n");
Is that possible? If so, we should discuss how that could happen in the commentary of the code.
No, I don't think it should be. Changed to assert() to clarify that.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/14/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/14/util/cbfstool/cbfstool.c@6... PS14, Line 673: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/16/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/16/util/cbfstool/cbfstool.c@6... PS16, Line 673: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41121/17/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/17/util/cbfstool/cbfstool.c@8... PS17, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 18:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/3c9e55f9_97e2ed74 PS18, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 19:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/4aa80a6b_c440b20c PS19, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 20:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/2a256e25_9e0b462b PS20, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 21:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/5fd2785e_d40aee9f PS21, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 21:
(1 comment)
File util/cbfstool/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/41121/comment/89d5f234_e85c1655 PS21, Line 15: buffer_size(&dev->buffer) - offset < size) AFAIUI, this check is to ensure `dev->buffer` is large enough to read `size` bytes at `offset`. Am I missing something, or could this be simplified?
if (buffer_size(&dev->buffer) < offset + size) return -1;
Attention is currently required from: Furquan Shaikh, Angel Pons. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 21:
(1 comment)
File util/cbfstool/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/41121/comment/368aaad0_13a4eb64 PS21, Line 15: buffer_size(&dev->buffer) - offset < size)
AFAIUI, this check is to ensure `dev->buffer` is large enough to read `size` bytes at `offset`. […]
Yes, it's essentially the same thing, but it's written slightly more complicated to be overflow-safe. In your example, if (offset + size) was larger than the max value that can fit in size_t, the check might not catch it.
Granted, for cbfstool this is less likely to be an issue, but it doesn't hurt to be careful.
Attention is currently required from: Furquan Shaikh, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 21: Code-Review+1
(1 comment)
File util/cbfstool/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/41121/comment/c1bc4273_58a10ef4 PS21, Line 15: buffer_size(&dev->buffer) - offset < size)
Yes, it's essentially the same thing, but it's written slightly more complicated to be overflow-safe […]
Ack
Attention is currently required from: Furquan Shaikh, Julius Werner. Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 21:
(1 comment)
Patchset:
PS21: It seems on !linux there have to be local changes to src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h: -#include <endian.h> +#include <commonlib/bsd/sysincludes.h>
And to util/cbfstool/cbfstool.c on line 4: +#define __BSD_VISIBLE 1
Attention is currently required from: Furquan Shaikh, Idwer Vollering. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 21:
(1 comment)
Patchset:
PS21:
It seems on !linux there have to be local changes to src/commonlib/bsd/include/commonlib/bsd/cbfs_pr […]
Right. I've uploaded a separate patch to fix that: CB:50630
Attention is currently required from: Furquan Shaikh, Idwer Vollering. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 22:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/a08fc09b_0c0e2020 PS22, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 23:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/5be4e706_dc1bc42c PS23, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 25:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/71de2fab_425d4ba0 PS25, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 26:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/95da0ad6_ca7f7edb PS26, Line 871: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 27:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/0c33de38_76ab1872 PS27, Line 872: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 28:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/7da312f6_008676e2 PS28, Line 872: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 29:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/41121/comment/bdbdcb5b_6ccfe90d PS29, Line 872: ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", line over 96 characters
Attention is currently required from: Furquan Shaikh, Idwer Vollering, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 29: Code-Review+2
(1 comment)
Patchset:
PS29: Without CBFS verification, the resulting coreboot.rom for Asrock B85M Pro4 does not change when building with BUILD_TIMELESS=1. Looks like no one has any objections.
Attention is currently required from: Furquan Shaikh, Idwer Vollering. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
Patch Set 29:
(1 comment)
Patchset:
PS29:
Without CBFS verification, the resulting coreboot. […]
And here I thought I'd have to sing Happy Birthday to this patch pretty soon. Thanks for releasing me! :)
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41121 )
Change subject: cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor ......................................................................
cbfstool: Support CONFIG_CBFS_VERIFICATION and metadata hash anchor
This patch adds support for the new CONFIG_CBFS_VERIFICATION feature to cbfstool. When CBFS verification is enabled, cbfstool must automatically add a hash attribute to every CBFS file it adds (with a handful of exceptions like bootblock and "header" pseudofiles that are never read by coreboot code itself). It must also automatically update the metadata hash that is embedded in the bootblock code. It will automatically find the metadata hash by scanning the bootblock for its magic number and use its presence to auto-detect whether CBFS verification is enabled for an image (and which hash algorithm to use).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I61a84add8654f60c683ef213b844a11b145a5cb7 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41121 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/cbfstool/Makefile.inc A util/cbfstool/cbfs_glue.h M util/cbfstool/cbfs_sections.h M util/cbfstool/cbfstool.c 4 files changed, 262 insertions(+), 19 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/util/cbfstool/Makefile.inc b/util/cbfstool/Makefile.inc index 4ef224c..7a13a79 100644 --- a/util/cbfstool/Makefile.inc +++ b/util/cbfstool/Makefile.inc @@ -23,6 +23,7 @@ cbfsobj += xdr.o cbfsobj += partitioned_file.o # COMMONLIB +cbfsobj += cbfs_private.o cbfsobj += fsp_relocate.o # FMAP cbfsobj += fmap.o @@ -94,6 +95,7 @@ TOOLCPPFLAGS ?= -D_DEFAULT_SOURCE # memccpy() from string.h TOOLCPPFLAGS += -D_BSD_SOURCE -D_SVID_SOURCE # _DEFAULT_SOURCE for older glibc TOOLCPPFLAGS += -D_XOPEN_SOURCE=700 # strdup() from string.h +TOOLCPPFLAGS += -D_GNU_SOURCE # memmem() from string.h TOOLCPPFLAGS += -I$(top)/util/cbfstool/flashmap TOOLCPPFLAGS += -I$(top)/util/cbfstool TOOLCPPFLAGS += -I$(objutil)/cbfstool @@ -199,6 +201,8 @@ $(objutil)/cbfstool/fmd_scanner.o: TOOLCFLAGS += -Wno-unused-function # Tolerate lzma sdk warnings $(objutil)/cbfstool/LzmaEnc.o: TOOLCFLAGS += -Wno-sign-compare -Wno-cast-qual +# Tolerate commonlib warnings +$(objutil)/cbfstool/cbfs_private.o: TOOLCFLAGS += -Wno-sign-compare # Tolerate lz4 warnings $(objutil)/cbfstool/lz4.o: TOOLCFLAGS += -Wno-missing-prototypes $(objutil)/cbfstool/lz4_wrapper.o: TOOLCFLAGS += -Wno-attributes diff --git a/util/cbfstool/cbfs_glue.h b/util/cbfstool/cbfs_glue.h new file mode 100644 index 0000000..11786be --- /dev/null +++ b/util/cbfstool/cbfs_glue.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _CBFS_GLUE_H_ +#define _CBFS_GLUE_H_ + +#include "cbfs_image.h" + +#define CBFS_ENABLE_HASHING 1 + +typedef const struct cbfs_image *cbfs_dev_t; + +static inline ssize_t cbfs_dev_read(cbfs_dev_t dev, void *buffer, size_t offset, size_t size) +{ + if (buffer_size(&dev->buffer) < offset || + buffer_size(&dev->buffer) - offset < size) + return -1; + + memcpy(buffer, buffer_get(&dev->buffer) + offset, size); + return size; +} + +static inline size_t cbfs_dev_size(cbfs_dev_t dev) +{ + return buffer_size(&dev->buffer); +} + +#endif /* _CBFS_GLUE_H_ */ diff --git a/util/cbfstool/cbfs_sections.h b/util/cbfstool/cbfs_sections.h index d7b0173..99a4761 100644 --- a/util/cbfstool/cbfs_sections.h +++ b/util/cbfstool/cbfs_sections.h @@ -10,6 +10,7 @@
#define SECTION_NAME_FMAP "FMAP" #define SECTION_NAME_PRIMARY_CBFS "COREBOOT" +#define SECTION_NAME_BOOTBLOCK "BOOTBLOCK"
#define SECTION_ANNOTATION_CBFS "CBFS"
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 72c01b9..c55838b 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -14,14 +14,14 @@ #include "cbfs_sections.h" #include "elfparsing.h" #include "partitioned_file.h" +#include <commonlib/bsd/cbfs_private.h> +#include <commonlib/bsd/metadata_hash.h> #include <commonlib/fsp.h> #include <commonlib/endian.h> #include <commonlib/helpers.h> #include <commonlib/region.h> #include <vboot_host.h>
-#define SECTION_WITH_FIT_TABLE "BOOTBLOCK" - struct command { const char *name; const char *optstring; @@ -116,6 +116,162 @@ .u64val = -1, };
+/* + * This "metadata_hash cache" caches the value and location of the CBFS metadata + * hash embedded in the bootblock when CBFS verification is enabled. The first + * call to get_mh_cache() searches for the cache by scanning the whole bootblock + * for its 8-byte signature, later calls will just return the previously found + * information again. If the cbfs_hash.algo member in the result is + * VB2_HASH_INVALID, that means no metadata hash was found and this image does + * not use CBFS verification. + */ +struct mh_cache { + const char *region; + size_t offset; + struct vb2_hash cbfs_hash; + bool initialized; +}; + +static struct mh_cache *get_mh_cache(void) +{ + static struct mh_cache mhc; + + if (mhc.initialized) + return &mhc; + + mhc.initialized = true; + + const struct fmap *fmap = partitioned_file_get_fmap(param.image_file); + if (!fmap) + goto no_metadata_hash; + + /* Find the bootblock. If there is a "BOOTBLOCK" FMAP section, it's + there. If not, it's a normal file in the primary CBFS section. */ + size_t offset, size; + struct buffer buffer; + if (fmap_find_area(fmap, SECTION_NAME_BOOTBLOCK)) { + if (!partitioned_file_read_region(&buffer, param.image_file, + SECTION_NAME_BOOTBLOCK)) + goto no_metadata_hash; + mhc.region = SECTION_NAME_BOOTBLOCK; + offset = 0; + size = buffer.size; + } else { + struct cbfs_image cbfs; + struct cbfs_file *bootblock; + if (!partitioned_file_read_region(&buffer, param.image_file, + SECTION_NAME_PRIMARY_CBFS)) + goto no_metadata_hash; + mhc.region = SECTION_NAME_PRIMARY_CBFS; + if (cbfs_image_from_buffer(&cbfs, &buffer, param.headeroffset)) + goto no_metadata_hash; + bootblock = cbfs_get_entry(&cbfs, "bootblock"); + if (!bootblock || ntohl(bootblock->type) != CBFS_TYPE_BOOTBLOCK) + goto no_metadata_hash; + offset = (void *)bootblock + ntohl(bootblock->offset) - + buffer_get(&cbfs.buffer); + size = ntohl(bootblock->len); + } + + /* Find and validate the metadata hash anchor inside the bootblock and + record its exact byte offset from the start of the FMAP region. */ + struct metadata_hash_anchor *anchor = memmem(buffer_get(&buffer) + offset, + size, METADATA_HASH_ANCHOR_MAGIC, sizeof(anchor->magic)); + if (anchor) { + if (!vb2_digest_size(anchor->cbfs_hash.algo)) { + ERROR("Unknown CBFS metadata hash type: %d\n", + anchor->cbfs_hash.algo); + goto no_metadata_hash; + } + mhc.cbfs_hash = anchor->cbfs_hash; + mhc.offset = (void *)anchor - buffer_get(&buffer); + return &mhc; + } + +no_metadata_hash: + mhc.cbfs_hash.algo = VB2_HASH_INVALID; + return &mhc; +} + +static void update_and_info(const char *name, void *dst, void *src, size_t size) +{ + if (!memcmp(dst, src, size)) + return; + char *src_str = bintohex(src, size); + char *dst_str = bintohex(dst, size); + INFO("Updating %s from %s to %s\n", name, dst_str, src_str); + memcpy(dst, src, size); + free(src_str); + free(dst_str); +} + +static int update_anchor(struct mh_cache *mhc, uint8_t *fmap_hash) +{ + struct buffer buffer; + if (!partitioned_file_read_region(&buffer, param.image_file, + mhc->region)) + return -1; + struct metadata_hash_anchor *anchor = buffer_get(&buffer) + mhc->offset; + /* The metadata hash anchor should always still be where we left it. */ + assert(!memcmp(anchor->magic, METADATA_HASH_ANCHOR_MAGIC, + sizeof(anchor->magic)) && + anchor->cbfs_hash.algo == mhc->cbfs_hash.algo); + update_and_info("CBFS metadata hash", anchor->cbfs_hash.raw, + mhc->cbfs_hash.raw, vb2_digest_size(anchor->cbfs_hash.algo)); + if (fmap_hash) { + update_and_info("FMAP hash", + metadata_hash_anchor_fmap_hash(anchor), fmap_hash, + vb2_digest_size(anchor->cbfs_hash.algo)); + } + if (!partitioned_file_write_region(param.image_file, &buffer)) + return -1; + return 0; + +} + +/* This should be called after every time CBFS metadata might have changed. It + will recalculate and update the metadata hash in the bootblock if needed. */ +static int maybe_update_metadata_hash(struct cbfs_image *cbfs) +{ + if (strcmp(param.region_name, SECTION_NAME_PRIMARY_CBFS)) + return 0; /* Metadata hash only embedded in primary CBFS. */ + + struct mh_cache *mhc = get_mh_cache(); + if (mhc->cbfs_hash.algo == VB2_HASH_INVALID) + return 0; + + cb_err_t err = cbfs_walk(cbfs, NULL, NULL, &mhc->cbfs_hash, + CBFS_WALK_WRITEBACK_HASH); + if (err != CB_CBFS_NOT_FOUND) { + ERROR("Unexpected cbfs_walk() error %d\n", err); + return -1; + } + + return update_anchor(mhc, NULL); +} + +/* This should be called after every time the FMAP or the bootblock itself might + have changed, and will write the new FMAP hash into the metadata hash anchor + in the bootblock if required (usually when the bootblock is first added). */ +static int maybe_update_fmap_hash(void) +{ + if (strcmp(param.region_name, SECTION_NAME_BOOTBLOCK) && + strcmp(param.region_name, SECTION_NAME_FMAP) && + param.type != CBFS_TYPE_BOOTBLOCK) + return 0; /* FMAP and bootblock didn't change. */ + + struct mh_cache *mhc = get_mh_cache(); + if (mhc->cbfs_hash.algo == VB2_HASH_INVALID) + return 0; + + uint8_t fmap_hash[VB2_MAX_DIGEST_SIZE]; + const struct fmap *fmap = partitioned_file_get_fmap(param.image_file); + if (!fmap || vb2_digest_buffer((const void *)fmap, fmap_size(fmap), + mhc->cbfs_hash.algo, fmap_hash, sizeof(fmap_hash))) + return -1; + return update_anchor(mhc, fmap_hash); +} + static bool region_is_flashmap(const char *region) { return partitioned_file_region_check_magic(param.image_file, region, @@ -451,13 +607,21 @@
header = cbfs_create_file_header(CBFS_TYPE_RAW, buffer.size, name); + + enum vb2_hash_algorithm algo = get_mh_cache()->cbfs_hash.algo; + if (algo != VB2_HASH_INVALID) + if (cbfs_add_file_hash(header, &buffer, algo)) { + ERROR("couldn't add hash for '%s'\n", name); + goto done; + } + if (cbfs_add_entry(&image, &buffer, offset, header, 0) != 0) { ERROR("Failed to add %llu into ROM image as '%s'.\n", (long long unsigned)u64val, name); goto done; }
- ret = 0; + ret = maybe_update_metadata_hash(&image);
done: free(header); @@ -564,6 +728,7 @@ h->offset = htonl(offset); h->architecture = htonl(CBFS_ARCHITECTURE_UNKNOWN);
+ /* Never add a hash attribute to the master header. */ header = cbfs_create_file_header(CBFS_TYPE_CBFSHEADER, buffer_size(&buffer), name); if (cbfs_add_entry(&image, &buffer, 0, header, 0) != 0) { @@ -594,7 +759,7 @@ return 1; }
- ret = 0; + ret = maybe_update_metadata_hash(&image);
done: free(header); @@ -692,18 +857,32 @@
if (convert && convert(&buffer, &offset, header) != 0) { ERROR("Failed to parse file '%s'.\n", filename); - buffer_delete(&buffer); - return 1; + goto error; }
- if (param.hash != VB2_HASH_INVALID) - if (cbfs_add_file_hash(header, &buffer, param.hash) == -1) { - ERROR("couldn't add hash for '%s'\n", name); - free(header); - buffer_delete(&buffer); - return 1; + /* Bootblock and CBFS header should never have file hashes. When adding + the bootblock it is important that we *don't* look up the metadata + hash yet (before it is added) or we'll cache an outdated result. */ + if (type != CBFS_TYPE_BOOTBLOCK && type != CBFS_TYPE_CBFSHEADER) { + enum vb2_hash_algorithm mh_algo = get_mh_cache()->cbfs_hash.algo; + if (mh_algo != VB2_HASH_INVALID && param.hash != mh_algo) { + if (param.hash == VB2_HASH_INVALID) { + param.hash = mh_algo; + } else { + ERROR("Cannot specify hash %s that's different from metadata hash algorithm %s\n", + vb2_get_hash_algorithm_name(param.hash), + vb2_get_hash_algorithm_name(mh_algo)); + goto error; + } }
+ if (param.hash != VB2_HASH_INVALID && + cbfs_add_file_hash(header, &buffer, param.hash) == -1) { + ERROR("couldn't add hash for '%s'\n", name); + goto error; + } + } + if (param.autogen_attr) { /* Add position attribute if assigned */ if (param.baseaddress_assigned || param.stage_xip) { @@ -767,14 +946,18 @@
if (cbfs_add_entry(&image, &buffer, offset, header, len_align) != 0) { ERROR("Failed to add '%s' into ROM image.\n", filename); - free(header); - buffer_delete(&buffer); - return 1; + goto error; }
free(header); buffer_delete(&buffer); - return 0; + + return maybe_update_metadata_hash(&image) || maybe_update_fmap_hash(); + +error: + free(header); + buffer_delete(&buffer); + return 1; }
static int cbfstool_convert_raw(struct buffer *buffer, @@ -1118,7 +1301,7 @@ return 1; }
- return 0; + return maybe_update_metadata_hash(&image); }
static int cbfs_create(void) @@ -1289,6 +1472,33 @@ cbfs_print_directory(&image); }
+ if (verbose) { + struct mh_cache *mhc = get_mh_cache(); + if (mhc->cbfs_hash.algo == VB2_HASH_INVALID) + return 0; + + struct vb2_hash real_hash = { .algo = mhc->cbfs_hash.algo }; + cb_err_t err = cbfs_walk(&image, NULL, NULL, &real_hash, + CBFS_WALK_WRITEBACK_HASH); + if (err != CB_CBFS_NOT_FOUND) { + ERROR("Unexpected cbfs_walk() error %d\n", err); + return 1; + } + char *hash_str = bintohex(real_hash.raw, + vb2_digest_size(real_hash.algo)); + printf("[METADATA HASH]\t%s:%s", + vb2_get_hash_algorithm_name(real_hash.algo), hash_str); + if (!strcmp(param.region_name, SECTION_NAME_PRIMARY_CBFS)) { + if (!memcmp(mhc->cbfs_hash.raw, real_hash.raw, + vb2_digest_size(real_hash.algo))) + printf(":valid"); + else + printf(":invalid"); + } + printf("\n"); + free(hash_str); + } + return 0; }
@@ -1387,7 +1597,8 @@ memcpy(param.image_region->data + offset, new_content.data, new_content.size); buffer_delete(&new_content); - return 0; + + return maybe_update_fmap_hash(); }
static int cbfs_read(void) @@ -1692,7 +1903,7 @@ "Find a place for a file of that size\n" " layout [-w] " "List mutable (or, with -w, readable) image regions\n" - " print [-r image,regions] " + " print [-r image,regions] [-k] " "Show the contents of the ROM\n" " extract [-r image,regions] [-m ARCH] -n NAME -f FILE [-U] " "Extracts a file from ROM\n"
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Idwer Vollering (Code Review)
-
Julius Werner (Code Review)