From 66df100930d9259238a402db3fe368b65647a41b Mon Sep 17 00:00:00 2001 From: Jeremy Compostella Date: Mon, 23 Oct 2023 13:00:33 -0700 Subject: cbfstool: Fix CBFS header buffer overflow In the unlikely but possible event where the name of the CBFS file is longer than 232 characters, `cbfs_create_file_header()' would overflow the buffer it allocated when it copies the CBFS filename. Change-Id: If1825b5af21f7a20ce2a7ccb2d45b195c2fb67b0 Signed-off-by: Jeremy Compostella Reviewed-on: https://review.coreboot.org/c/coreboot/+/78500 Tested-by: build bot (Jenkins) Reviewed-by: Eric Lai Reviewed-by: Julius Werner --- util/cbfstool/cbfs_image.c | 36 ++++++++++++++++++++++++------------ util/cbfstool/cbfstool.c | 6 ++++++ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 7a34bfe58f04..2842252585c4 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -401,8 +401,8 @@ int cbfs_copy_instance(struct cbfs_image *image, struct buffer *dst) if (last_entry_size < 0) WARN("No room to create the last entry!\n"); else - cbfs_create_empty_entry(dst_entry, CBFS_TYPE_NULL, - last_entry_size, ""); + return cbfs_create_empty_entry(dst_entry, CBFS_TYPE_NULL, + last_entry_size, ""); return 0; } @@ -437,8 +437,10 @@ int cbfs_expand_to_region(struct buffer *region) cbfs_calculate_file_header_size("") - sizeof(int32_t); if (last_entry_size > 0) { - cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, - last_entry_size, ""); + if (cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, + last_entry_size, "")) + return 1; + /* If the last entry was an empty file, merge them. */ cbfs_legacy_walk(&image, cbfs_merge_empty_entry, NULL); } @@ -586,8 +588,9 @@ int cbfs_compact_instance(struct cbfs_image *image) prev_size -= spill_size + empty_metadata_size; /* Create new empty file. */ - cbfs_create_empty_entry(cur, CBFS_TYPE_NULL, - prev_size, ""); + if (cbfs_create_empty_entry(cur, CBFS_TYPE_NULL, + prev_size, "")) + return 1; /* Merge any potential empty entries together. */ cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL); @@ -641,7 +644,8 @@ static int cbfs_add_entry_at(struct cbfs_image *image, if (header_offset - addr > min_entry_size) { DEBUG("|min|...|header|content|... \n"); len = header_offset - addr - min_entry_size; - cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, len, ""); + if (cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, len, "")) + return -1; if (verbose > 1) cbfs_print_entry_info(image, entry, stderr); entry = cbfs_find_next_entry(image, entry); addr = cbfs_get_entry_addr(image, entry); @@ -713,7 +717,8 @@ static int cbfs_add_entry_at(struct cbfs_image *image, buffer_size(&image->buffer) - sizeof(int32_t)) { len -= sizeof(int32_t); } - cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, len, ""); + if (cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, len, "")) + return -1; if (verbose > 1) cbfs_print_entry_info(image, entry, stderr); return 0; } @@ -1650,9 +1655,7 @@ int cbfs_merge_empty_entry(struct cbfs_image *image, struct cbfs_file *entry, uint32_t addr = cbfs_get_entry_addr(image, entry); size_t len = next_addr - addr - cbfs_calculate_file_header_size(""); DEBUG("join_empty_entry: [0x%x, 0x%x) len=%zu\n", addr, next_addr, len); - cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, len, ""); - - return 0; + return cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, len, ""); } int cbfs_legacy_walk(struct cbfs_image *image, cbfs_entry_callback callback, @@ -1785,13 +1788,19 @@ int cbfs_is_valid_entry(struct cbfs_image *image, struct cbfs_file *entry) struct cbfs_file *cbfs_create_file_header(int type, size_t len, const char *name) { + size_t header_size = cbfs_calculate_file_header_size(name); + if (header_size > CBFS_METADATA_MAX_SIZE) { + ERROR("'%s' name too long to fit in CBFS header\n", name); + return NULL; + } + struct cbfs_file *entry = malloc(CBFS_METADATA_MAX_SIZE); memset(entry, CBFS_CONTENT_DEFAULT_VALUE, CBFS_METADATA_MAX_SIZE); memcpy(entry->magic, CBFS_FILE_MAGIC, sizeof(entry->magic)); entry->type = htobe32(type); entry->len = htobe32(len); entry->attributes_offset = 0; - entry->offset = htobe32(cbfs_calculate_file_header_size(name)); + entry->offset = htobe32(header_size); memset(entry->filename, 0, be32toh(entry->offset) - sizeof(*entry)); strcpy(entry->filename, name); return entry; @@ -1801,6 +1810,9 @@ int cbfs_create_empty_entry(struct cbfs_file *entry, int type, size_t len, const char *name) { struct cbfs_file *tmp = cbfs_create_file_header(type, len, name); + if (!tmp) + return -1; + memcpy(entry, tmp, be32toh(tmp->offset)); free(tmp); memset(CBFS_SUBHEADER(entry), CBFS_CONTENT_DEFAULT_VALUE, len); diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 3df7b520899c..e11cfbc12641 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -650,6 +650,8 @@ static int cbfs_add_integer_component(const char *name, header = cbfs_create_file_header(CBFS_TYPE_RAW, buffer.size, name); + if (!header) + goto done; enum vb2_hash_algorithm algo = get_mh_cache()->cbfs_hash.algo; if (algo != VB2_HASH_INVALID) @@ -774,6 +776,8 @@ static int cbfs_add_master_header(void) /* Never add a hash attribute to the master header. */ header = cbfs_create_file_header(CBFS_TYPE_CBFSHEADER, buffer_size(&buffer), name); + if (!header) + goto done; if (cbfs_add_entry(&image, &buffer, 0, header, 0) != 0) { ERROR("Failed to add cbfs master header into ROM image.\n"); goto done; @@ -915,6 +919,8 @@ static int cbfs_add_component(const char *filename, struct cbfs_file *header = cbfs_create_file_header(param.type, buffer.size, name); + if (!header) + goto error; /* Bootblock and CBFS header should never have file hashes. When adding the bootblock it is important that we *don't* look up the metadata -- cgit v1.2.3