GNUBoot
/
coreboot-kgpe-d16
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

commonlib/cbfs: Fix minor parser edge cases 

This patch fixes a few minor CBFS parsing edge cases that could lead to
unintended behavior: the CBFS attribute parser could have run into an
infinite loop if an attribute's length was (accidentally or maliciously)
invalid. A length of 0 would have caused it to read the same attribute
over and over again without making forward progress, while a very large
length could have caused an overflow that makes it go backwards to find
the next attribute. Also, the filename was not guaranteed to be
null-terminated which could have resulted in out-of-bounds reads on a
few error messages.

Finally, clarify the validity guarantees for CBFS header fields offered
by cbfs_walk() in the comment explaining cbfs_mdata.

Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: Ie569786e5bec355b522f6580f53bdd8b16a4d726
Reviewed-on: https://review.coreboot.org/c/coreboot/+/57569
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Jakub Czapiga <jacz@semihalf.com>
This commit is contained in:
Julius Werner 2021-09-10 17:14:41 -07:00
parent a472c54a63
commit a4c0e60725
2 changed files with 9 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/commonlib/bsd/cbfs_private.c
View File

@ -78,6 +78,8 @@ cb_err_t cbfs_walk(cbfs_dev_t dev, cb_err_t (*walker)(cbfs_dev_t dev, size_t off
if (cbfs_dev_read(dev, mdata.raw + sizeof(mdata.h),
offset + sizeof(mdata.h), todo) != todo)
return CB_CBFS_IO;
/* Force filename null-termination, just in case. */
mdata.raw[attr_offset ? attr_offset - 1 : data_offset - 1] = '\0';
DEBUG("File name: '%s'\n", mdata.h.filename);
if (do_hash && !empty && vb2_digest_extend(&dc, mdata.raw, data_offset))
@ -175,9 +177,9 @@ const void *cbfs_find_attr(const union cbfs_mdata *mdata, uint32_t attr_tag, siz
const uint32_t tag = be32toh(attr->tag);
const uint32_t len = be32toh(attr->len);
if (offset + len > end) {
ERROR("Attribute %s[%x] overflows end of metadata\n",
mdata->h.filename, tag);
if (len < sizeof(struct cbfs_file_attribute) || len > end - offset) {
ERROR("Attribute %s[%x] invalid length: %u\n",
mdata->h.filename, tag, len);
return NULL;
}
if (tag == attr_tag) {

5
src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h
View File

@ -8,7 +8,10 @@
#include <stdint.h>
/*
* Helper structure to allocate space for a blob of metadata on the stack.
* Helper structure to allocate space for a blob of metadata on the stack. All functions using
* a cbfs_mdata should be getting it via cbfs_walk(), and can rely on the fact that cbfs_walk()
* has already fully validated the header (range checks for `len`, `attributes_offset` and
* `offset`, and null-termination for `filename`).
* NOTE: The fields in any union cbfs_mdata or any of its substructures from cbfs_serialized.h
* should always remain in the same byte order as they are stored on flash (= big endian). To
* avoid byte-order confusion, fields should always and only be converted to host byte order at