Hello Furquan Shaikh, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48838
to review the following change.
Change subject: cbfs: Add file data hashing to cbfs_load() and cbfs_map() ......................................................................
cbfs: Add file data hashing to cbfs_load() and cbfs_map()
This patch adds the file data hashing part of CONFIG_CBFS_VERIFICATION to the cbfs_load() and cbfs_map() family of functions. To facilitate this, cbfs_load_and_decompress() is expanded to take an optional file_hash attribute that will be verified between loading and decompression. Note that hashing for other file accesses (such as stage loading) are not yet supported and will be added in later patches.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Icdf6f73489f16f507435857e810495d1034edec9 --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/drivers/intel/fsp2_0/util.c M src/include/cbfs.h M src/lib/cbfs.c M src/lib/rmodule.c 6 files changed, 85 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/48838/1
diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c index 91f81e0..8d11b48 100644 --- a/src/commonlib/bsd/cbfs_private.c +++ b/src/commonlib/bsd/cbfs_private.c @@ -190,3 +190,26 @@
return NULL; } + +const struct vb2_hash *cbfs_file_hash(const union cbfs_mdata *mdata) +{ + /* Hashes are variable-length attributes, so need to manually check the length. */ + const struct cbfs_file_attr_hash *attr = + cbfs_find_attr(mdata, CBFS_FILE_ATTR_TAG_HASH, 0); + if (!attr) + return NULL; /* no hash */ + const size_t asize = be32toh(attr->len); + + const struct vb2_hash *hash = &attr->hash; + const size_t hsize = vb2_digest_size(hash->algo); + if (!hsize) { + ERROR("Hash algo for '%s' unsupported. (%u)\n", mdata->h.filename, hash->algo); + return NULL; + } + if (hsize != asize - offsetof(struct cbfs_file_attr_hash, hash.raw)) { + ERROR("Hash attribute size for '%s' (%zu) incorrect for algo %u.\n", + mdata->h.filename, asize, hash->algo); + return NULL; + } + return hash; +} diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h index 9fe740c9..f8d1a30 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h @@ -135,4 +135,7 @@ else caller is responsible for checking the |len| field to avoid reading out-of-bounds. */ const void *cbfs_find_attr(const union cbfs_mdata *mdata, uint32_t attr_tag, size_t size_check);
+/* Returns pointer to CBFS file hash structure in metadata attributes, or NULL if invalid. */ +const struct vb2_hash *cbfs_file_hash(const union cbfs_mdata *mdata); + #endif /* _COMMONLIB_BSD_CBFS_PRIVATE_H_ */ diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c index e35b378..26de3e7 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -154,7 +154,7 @@ if (fspm_xip()) return dest;
- if (cbfs_load_and_decompress(source_rdev, dest, size, compression_algo) != size) { + if (cbfs_load_and_decompress(source_rdev, dest, size, compression_algo, NULL) != size) { printk(BIOS_ERR, "Failed to load FSP component.\n"); return NULL; } diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 18204a7..dde4876 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -42,12 +42,14 @@ ("COREBOOT" FMAP region), even when CONFIG(VBOOT) is enabled. */ size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size); /* Load all bytes from |rdev| to the |buffer_size| bytes large |buffer|, - * decompressing it according to |compression| in the process. + * decompressing it according to |compression| in the process. If a |file_hash| + * is passed, use it to verify the data before decompression. * Returns the decompressed file size, or 0 on error. * LZMA files will be mapped for decompression. LZ4 files will be decompressed * in-place with the buffer size requirements outlined in compression.h. */ size_t cbfs_load_and_decompress(const struct region_device *rdev, void *buffer, - size_t buffer_size, uint32_t compression); + size_t buffer_size, uint32_t compression, + const struct vb2_hash *file_hash);
/* Load stage into memory filling in prog. Return 0 on success. < 0 on error. */ int cbfs_prog_stage_load(struct prog *prog); diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 3684bde..bfbb7bc 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -95,6 +95,21 @@ return 0; }
+static inline bool cbfs_file_hash_mismatch(const void *buffer, size_t size, + const struct vb2_hash *file_hash) +{ + /* Avoid linking hash functions when verification is disabled. */ + if (!CONFIG(CBFS_VERIFICATION)) + return false; + + /* If there is no file hash, always count that as a mismatch. */ + if (file_hash && vb2_hash_verify(buffer, size, file_hash) == VB2_SUCCESS) + return false; + + printk(BIOS_CRIT, "CBFS file hash mismatch!\n"); + return true; +} + static void *_cbfs_map(const char *name, size_t *size_out, bool force_ro) { struct region_device rdev; @@ -106,7 +121,18 @@ if (size_out != NULL) *size_out = region_device_sz(&rdev);
- return rdev_mmap_full(&rdev); + void *mapping = rdev_mmap_full(&rdev); + + if (CONFIG(CBFS_VERIFICATION)) { + const struct vb2_hash *hash = cbfs_file_hash(&mdata); + if (cbfs_file_hash_mismatch(mapping, region_device_sz(&rdev), + hash)) { + rdev_munmap(&rdev, mapping); + return NULL; + } + } + + return mapping; }
void *cbfs_map(const char *name, size_t *size_out) @@ -195,7 +221,8 @@ }
size_t cbfs_load_and_decompress(const struct region_device *rdev, void *buffer, - size_t buffer_size, uint32_t compression) + size_t buffer_size, uint32_t compression, + const struct vb2_hash *file_hash) { size_t in_size = region_device_sz(rdev); size_t out_size; @@ -207,6 +234,8 @@ return 0; if (rdev_readat(rdev, buffer, 0, in_size) != in_size) return 0; + if (cbfs_file_hash_mismatch(buffer, in_size, file_hash)) + return 0; return in_size;
case CBFS_COMPRESS_LZ4: @@ -218,6 +247,8 @@ map = rdev_mmap_full(rdev); if (map == NULL) return 0; + if (cbfs_file_hash_mismatch(map, in_size, file_hash)) + return 0;
timestamp_add_now(TS_START_ULZ4F); out_size = ulz4fn(map, in_size, buffer, buffer_size); @@ -233,6 +264,8 @@ map = rdev_mmap_full(rdev); if (map == NULL) return 0; + if (cbfs_file_hash_mismatch(map, in_size, file_hash)) + return 0;
/* Note: timestamp not useful for memory-mapped media (x86) */ timestamp_add_now(TS_START_ULZMA); @@ -249,7 +282,8 @@ }
static size_t cbfs_stage_load_and_decompress(const struct region_device *rdev, - void *buffer, size_t buffer_size, uint32_t compression) + void *buffer, size_t buffer_size, + uint32_t compression, const struct vb2_hash *file_hash) { size_t in_size = region_device_sz(rdev); struct region_device rdev_src; @@ -268,12 +302,13 @@ rdev_chain(&rdev_src, &addrspace_32bit.rdev, (uintptr_t)compr_start, in_size);
- return cbfs_load_and_decompress(&rdev_src, buffer, - buffer_size, compression); + return cbfs_load_and_decompress(&rdev_src, buffer, buffer_size, + compression, file_hash); }
/* All other algorithms can use the generic implementation. */ - return cbfs_load_and_decompress(rdev, buffer, buffer_size, compression); + return cbfs_load_and_decompress(rdev, buffer, buffer_size, + compression, file_hash); }
static inline int tohex4(unsigned int c) @@ -326,15 +361,20 @@ return 0;
uint32_t compression = CBFS_COMPRESS_NONE; - const struct cbfs_file_attr_compression *attr = cbfs_find_attr(&mdata, - CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*attr)); - if (attr) { - compression = be32toh(attr->compression); - if (buf_size < be32toh(attr->decompressed_size)) + const struct cbfs_file_attr_compression *cattr = cbfs_find_attr(&mdata, + CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*cattr)); + if (cattr) { + compression = be32toh(cattr->compression); + if (buf_size < be32toh(cattr->decompressed_size)) return 0; }
- return cbfs_load_and_decompress(&rdev, buf, buf_size, compression); + const struct vb2_hash *file_hash = NULL; + if (CONFIG(CBFS_VERIFICATION)) + file_hash = cbfs_file_hash(&mdata); + + return cbfs_load_and_decompress(&rdev, buf, buf_size, + compression, file_hash); }
size_t cbfs_load(const char *name, void *buf, size_t buf_size) @@ -364,7 +404,7 @@
fsize = cbfs_stage_load_and_decompress(rdev, prog_start(pstage), prog_size(pstage), - prog_compression(pstage)); + prog_compression(pstage), NULL); if (!fsize) return -1;
diff --git a/src/lib/rmodule.c b/src/lib/rmodule.c index d8cfd36..bcd7b18 100644 --- a/src/lib/rmodule.c +++ b/src/lib/rmodule.c @@ -263,7 +263,7 @@ prog_name(rsl->prog), rmod_loc, prog_size(prog));
if (!cbfs_load_and_decompress(prog_rdev(prog), rmod_loc, - prog_size(prog), prog_compression(prog))) + prog_size(prog), prog_compression(prog), NULL)) return -1;
if (rmodule_parse(rmod_loc, &rmod_stage))
Julius Werner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48838 )
Change subject: cbfs: Add file data hashing to cbfs_load() and cbfs_map() ......................................................................
Abandoned
Superseded by CB:52084
-
Julius Werner (Code Review)