Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38421
to review the following change.
Change subject: commonlib/bsd: Add new CBFS core implementation ......................................................................
commonlib/bsd: Add new CBFS core implementation
This patch adds a new CBFS implementation that is intended to replace the existing commonlib/cbfs.c. The new implementation is designed to meet a bunch of current and future goals that in aggregate make it easier to start from scratch than to adapt the exisiting implementation:
1. Be BSD-licensed so it can evetually be shared with libpayload. 2. Allow generating/verifying a master hash for future CBFS per-file verification (see [1][2]). 3. Be very careful about reading (not mmaping) all data only once, to be suitable for eventual TOCTOU-safe verification. 4. Make it possibly to efficiently implement all current and future firmware use cases (both with and without verification).
The main primitive is the cbfs_walk() function which will traverse a CBFS and call a callback for every file. cbfs_lookup() uses this to implement the most common use case of finding a file so that it can be read. A host application using this code (e.g. coreboot, libpayload, cbfstool) will need to provide a <cbfs_glue.h> header to provide the glue to access the respective CBFS storage backend implementation.
This patch merely adds the code, the next patch will integrate it into coreboot.
[1]: https://www.youtube.com/watch?v=Hs_EhewBgtM [2]: https://osfc.io/uploads/talk/paper/47/The_future_of_firmware_verification_in...
Change-Id: Ica64c1751fa37686814c0247460c399261d5814c Signed-off-by: Julius Werner jwerner@chromium.org --- M MAINTAINERS A src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cb_err.h A src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h 5 files changed, 284 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/38421/1
diff --git a/MAINTAINERS b/MAINTAINERS index 77769c0..b5cad10 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -489,8 +489,13 @@ F: src/device/oprom/
CBFS -F: src/include/cbfs.h -F: src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h +M: Julius Werner jwerner@chromium.org +F: src/include/cbfs* +F: src/commonlib/bsd/include/commonlib/bsd/cbfs* +F: src/commonlib/bsd/cbfs* +F: src/lib/cbfs.c + +CBFSTOOL F: util/cbfstool/
CBMEM diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c new file mode 100644 index 0000000..4dd14cb --- /dev/null +++ b/src/commonlib/bsd/cbfs_private.c @@ -0,0 +1,174 @@ +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-or-later */ + +#include <commonlib/bsd/cbfs_private.h> +#include <assert.h> + +static cb_err_t read_next_header(cbfs_dev_t dev, size_t *offset, struct cbfs_file *buffer) +{ + DEBUG("Looking for next file @%#zx...\n", *offset); + *offset = ALIGN_UP(*offset, CBFS_ALIGNMENT); + while (*offset + sizeof(*buffer) < cbfs_dev_size(dev)) { + if (cbfs_dev_read(dev, buffer, *offset, sizeof(*buffer)) != sizeof(*buffer)) + return CB_CBFS_IO; + + if (memcmp(buffer->magic, CBFS_FILE_MAGIC, sizeof(buffer->magic)) == 0) + return CB_SUCCESS; + + *offset += CBFS_ALIGNMENT; + } + + DEBUG("End of CBFS reached\n"); + return CB_CBFS_NOT_FOUND; +} + +cb_err_t cbfs_walk(cbfs_dev_t dev, cb_err_t (*walker)(cbfs_dev_t dev, size_t offset, + union cbfs_mdata *mdata, bool do_hash, + void *arg), + void *arg, struct vb2_hash *master_hash, enum cbfs_walk_flags flags) +{ + bool do_hash = ENABLE_HASHING && master_hash; + struct vb2_digest_context dc; + vb2_error_t vbrv; + + assert(ENABLE_HASHING || (!master_hash && !(flags & CBFS_WALK_WRITEBACK_HASH))); + if (do_hash && (vbrv = vb2_digest_init(&dc, master_hash->algo))) { + ERROR("Master hash algo %d digest init error: %#x\n", master_hash->algo, vbrv); + return CB_ERR_ARG; + } + + size_t offset = 0; + cb_err_t ret_header; + cb_err_t ret_walker = CB_CBFS_NOT_FOUND; + union cbfs_mdata mdata; + while ((ret_header = read_next_header(dev, &offset, &mdata.h)) == CB_SUCCESS) { + const uint32_t attr_offset = be32toh(mdata.h.attributes_offset); + const uint32_t data_offset = be32toh(mdata.h.offset); + const uint32_t data_length = be32toh(mdata.h.len); + const uint32_t type = be32toh(mdata.h.type); + const bool empty = (type == CBFS_TYPE_DELETED || type == CBFS_TYPE_DELETED2); + + DEBUG("Found CBFS header @%#zx (type %d, attr +%#x, data +%#x, length %#x)\n", + offset, be32toh(mdata.h.type), attr_offset, data_offset, data_length); + if (data_offset > sizeof(mdata)) { + ERROR("File metadata @%#zx too large\n", offset); + goto next_file; + } + if (offset + data_offset >= cbfs_dev_size(dev)) { + ERROR("Device ends in middle of metadata @%#zx\n", offset); + goto next_file; + } + + if (empty && !(flags & CBFS_WALK_INCLUDE_EMPTY)) + goto next_file; + + /* When hashing we need to read everything. Otherwise skip the attributes. + attr_offset may be 0, which means there are no attributes. */ + ssize_t todo; + if (do_hash || attr_offset == 0) + todo = data_offset - sizeof(mdata.h); + else + todo = attr_offset - sizeof(mdata.h); + if (todo <= 0 || data_offset < attr_offset) { + ERROR("Corrupt file header @%#zx\n", offset); + goto next_file; + } + + /* Read the rest of the metadata (filename, and possibly attributes). */ + if (cbfs_dev_read(dev, (uint8_t *)&mdata + sizeof(mdata.h), + offset + sizeof(mdata.h), todo) != todo) + return CB_CBFS_IO; + DEBUG("File name: '%s'\n", mdata.filename); + + if (do_hash && !empty && vb2_digest_extend(&dc, (void *)&mdata, data_offset)) + return CB_ERR; + + if (ret_walker == CB_CBFS_NOT_FOUND) + ret_walker = walker(dev, offset, &mdata, do_hash, arg); + + /* Return IO errors immediately. For others, finish the hash first if needed. */ + if (ret_walker == CB_CBFS_IO || (ret_walker != CB_CBFS_NOT_FOUND && !do_hash)) + return ret_walker; + +next_file: + offset += data_offset + data_length; + } + + if (ret_header != CB_CBFS_NOT_FOUND) + return ret_header; + + if (do_hash) { + uint8_t real_hash[VB2_MAX_DIGEST_SIZE]; + size_t hash_size = vb2_digest_size(master_hash->algo); + if (vb2_digest_finalize(&dc, real_hash, sizeof(real_hash))) + return CB_ERR; + if (flags & CBFS_WALK_WRITEBACK_HASH) + memcpy(master_hash->bytes.raw, real_hash, hash_size); + else if (memcmp(master_hash->bytes.raw, real_hash, hash_size) != 0) + return CB_CBFS_HASH; + } + + return ret_walker; +} + +cb_err_t cbfs_copy_fill_metadata(union cbfs_mdata *dst, union cbfs_mdata *src, + cbfs_dev_t dev, size_t offset, bool do_hash) +{ + const size_t attr_offset = be32toh(src->h.attributes_offset); + + /* In the hashing case or when there are no attributes, we already have everything. */ + if (do_hash || attr_offset == 0) { + memcpy(dst, src, be32toh(src->h.offset)); + return CB_SUCCESS; + } + + /* Otherwise we'll need to copy what we have and read in the rest. */ + const size_t todo = be32toh(src->h.offset) - attr_offset; + memcpy(dst, src, attr_offset); + void *dst_attrs = (uint8_t *)dst + attr_offset; + if (cbfs_dev_read(dev, dst_attrs, offset + attr_offset, todo) != todo) + return CB_CBFS_IO; + return CB_SUCCESS; +} + +uint32_t cbfs_filename_size(struct cbfs_file *h) +{ + uint32_t end_offset = be32toh(h->attributes_offset); + if (end_offset == 0) + end_offset = be32toh(h->offset); + return end_offset - sizeof(struct cbfs_file); +} + +struct cbfs_lookup_args { + union cbfs_mdata *mdata_out; + const char *name; + size_t namesize; + size_t *data_offset_out; +}; + +static cb_err_t lookup_walker(cbfs_dev_t dev, size_t offset, union cbfs_mdata *mdata, + bool do_hash, void *arg) +{ + struct cbfs_lookup_args *args = arg; + if (args->namesize > cbfs_filename_size(&mdata->h) || + memcmp(args->name, mdata->filename, args->namesize) != 0) + return CB_CBFS_NOT_FOUND; + + LOG("Found '%s' @%#zx size %#x\n", args->name, offset, be32toh(mdata->h.len)); + if (cbfs_copy_fill_metadata(args->mdata_out, mdata, dev, offset, do_hash) != CB_SUCCESS) + return CB_CBFS_IO; + + *args->data_offset_out = offset + be32toh(mdata->h.offset); + return CB_SUCCESS; +} + +cb_err_t cbfs_lookup(cbfs_dev_t dev, const char *name, union cbfs_mdata *mdata_out, + size_t *data_offset_out, struct vb2_hash *master_hash) +{ + struct cbfs_lookup_args args = { + .mdata_out = mdata_out, + .name = name, + .namesize = strlen(name) + 1, /* Count trailing \0 so we can memcmp() it. */ + .data_offset_out = data_offset_out, + }; + return cbfs_walk(dev, lookup_walker, &args, master_hash, 0); +} diff --git a/src/commonlib/bsd/include/commonlib/bsd/cb_err.h b/src/commonlib/bsd/include/commonlib/bsd/cb_err.h index ab419a7..ab422e1 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cb_err.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cb_err.h @@ -34,6 +34,11 @@ CB_I2C_PROTOCOL_ERROR = -302, /**< Data lost or spurious slave device response, try again? */ CB_I2C_TIMEOUT = -303, /**< Transmission timed out */ + + /* CBFS errors */ + CB_CBFS_IO = -400, /**< Underlying I/O error */ + CB_CBFS_NOT_FOUND = -401, /**< File not found in directory */ + CB_CBFS_HASH = -402, /**< Master hash validation failed */ };
/* Don't typedef the enum directly, so the size is unambiguous for serialization. */ diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h new file mode 100644 index 0000000..b4fc1c7 --- /dev/null +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h @@ -0,0 +1,95 @@ +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-or-later */ + +#ifndef _COMMONLIB_BSD_CBFS_PRIVATE_H_ +#define _COMMONLIB_BSD_CBFS_PRIVATE_H_ + +#include <commonlib/bsd/cb_err.h> +#include <commonlib/bsd/cbfs_serialized.h> +#include <endian.h> +#include <stdbool.h> +#include <stdint.h> +#include <vb2_sha.h> + +/* + * This file needs to be provided by the host application using this CBFS library. It must + * define the following type, macros and functions: + * + * cbfs_dev_t An opaque type representing a CBFS storage backend. + * ENABLE_HASHING Should be 0 to avoid linking hashing features, 1 otherwise. + * ERROR(...) printf-style macro to print errors. + * LOG(...) printf-style macro to print normal-operation log messages. + * DEBUG(...) printf-style macro to print detailed debug output. + * + * ssize_t cbfs_dev_read(cbfs_dev_t dev, void *buffer, size_t offset, size_t size); + * Read |size| bytes starting at |offset| from |dev| into |buffer|. + * Returns amount of bytes read on success and < 0 on error. + * + * size_t cbfs_dev_size(cbfs_dev_t dev); + * Return the total size in bytes of the CBFS storage (actual CBFS area). + */ +#include <cbfs_glue.h> + +/* Helper structure to allocate space for a blob of metadata on the stack. */ +#define CBFS_METADATA_MAX_SIZE 256 +union cbfs_mdata { + struct { + struct cbfs_file h; + char filename[]; + }; + uint8_t raw[CBFS_METADATA_MAX_SIZE]; +}; + +/* Flags that modify behavior of cbfs_walk(). */ +enum cbfs_walk_flags { + /* Write the calculated hash back out to |master_hash->hash|, rather than comparing it. + |master_hash->algo| must still have been initialized by the caller. */ + CBFS_WALK_WRITEBACK_HASH = (1 << 0), + /* Call |walker| for empty file entries. Otherwise, empty entries will be skipped. + Either way, empty entries are never included in master_hash calculation. */ + CBFS_WALK_INCLUDE_EMPTY = (1 << 1), +}; + +/* + * Traverse a CBFS and call a |walker| callback function for every file. Can additionally + * calculate a master hash over the metadata of all files in the CBFS. If |master_hash| is NULL, + * hashing is disabled. + * + * |arg| and |dev| will be passed through to |walker| unmodified. |do_hash| is true iff + * hashing is enabled (master_hash != NULL). |offset| is the total offset in |dev| at which the + * current file metadata starts. |mdata| is a temporary buffer (only valid for the duration of + * this call to |walker|) containing already read metadata from the current file: if |do_hash| + * is true, all metadata in |mdata| (header, filename and attributes) will be valid, otherwise + * only the header and the filename (not attributes) will be valid. |walker| should call into + * cbfs_copy_fill_medadata() to copy the metadata of a file to a persistent buffer and + * automatically load remaining attributes from |dev| as needed based on |do_hash|. + * + * |walker| should return CB_CBFS_NOT_FOUND if it wants to continue being called for further + * files. Any other return code will be used as the final return code for cbfs_walk(). It will + * return immediately unless it needs to calculate a hash in which case it will still traverse + * the remaining CBFS (but not call |walker| anymore). + */ +cb_err_t cbfs_walk(cbfs_dev_t dev, cb_err_t (*walker)(cbfs_dev_t dev, size_t offset, + union cbfs_mdata *mdata, bool do_hash, + void *arg), + void *arg, struct vb2_hash *master_hash, enum cbfs_walk_flags); + + +/* + * Helper function that can be used by a |walker| callback to cbfs_walk() to copy the metadata + * of a file into a permanent buffer. Will copy the metadata from |src| into |dst| and load + * additional attributes from |dev| to copy behind that if |do_hash| is false. (|offset| is the + * offset of the whole file metadata, not just of the attributes.) + */ +cb_err_t cbfs_copy_fill_metadata(union cbfs_mdata *dst, union cbfs_mdata *src, + cbfs_dev_t dev, size_t offset, bool do_hash); + +/* Helper function to determine the size of the filename part in a CBFS header. The actual + filename is NUL-terminated and due to alignment likely a bit shorter than this. */ +uint32_t cbfs_filename_size(struct cbfs_file *h); + +/* Find a file named |name| in the CBFS on |dev| and copy its metadata (including attributes) + * into |mdata_out|. Pass out offset to the file data and verify |master_hash| (if provided). */ +cb_err_t cbfs_lookup(cbfs_dev_t dev, const char *name, union cbfs_mdata *mdata_out, + size_t *data_offset_out, struct vb2_hash *master_hash); + +#endif /* _COMMONLIB_BSD_CBFS_PRIVATE_H_ */ diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h index d2fc626..556c8e4 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h @@ -4,6 +4,7 @@ #define _CBFS_SERIALIZED_H_
#include <stdint.h> +#include <vb2_sha.h>
/** These are standard values for the known compression algorithms that coreboot knows about for stages and @@ -123,12 +124,11 @@ uint32_t decompressed_size; } __packed;
+/* Actual size in CBFS may be larger/smaller than struct size! */ struct cbfs_file_attr_hash { uint32_t tag; uint32_t len; - uint32_t hash_type; - /* hash_data is len - sizeof(struct) bytes */ - uint8_t hash_data[]; + struct vb2_hash hash; } __packed;
struct cbfs_file_attr_position {