Change in coreboot[master]: commonlib/bsd: Add new CBFS core implementation - coreboot-gerrit

15 Jan 2020

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 {
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38421
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ica64c1751fa37686814c0247460c399261d5814c
Gerrit-Change-Number: 38421
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner jwerner@chromium.org
Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org
Gerrit-MessageType: newchange



    