Change in coreboot[master]: cbfs: Make sure all cases of single file header corruption are isolated

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/57271 )

Change subject: cbfs: Make sure all cases of single file header corruption are isolated
......................................................................

cbfs: Make sure all cases of single file header corruption are isolated

The new CBFS stack was written to try to isolate cases of single file
corruption as far as possible and still make other files avaialble (at
least as long as verification is disabled and they can still be found at
all). For most cases of header corruption, it will just continue trying
to parse the next file. However, in cases where parts of the file extend
beyond the end of the rdev, we have been relying on the range checking
of the rdev API rather than doing it explicitly.

This is fine in general, but it causes the problem that these errors
cannot be distinguished from I/O errors, and I/O errors always make the
whole cbfs_walk() fail. That means we will not return a successful
result from cbfs_mcache_build(), and leads to an odd discrepancy in how
certain kinds of corrupted CBFSes are treated with and without mcache.
This patch adds an explicit range check to make the behavior consistent.

Signed-off-by: Julius Werner 
Change-Id: Ice2b6960284bd0c19be35b0607e5e32791e7a64c
Reviewed-on: https://review.coreboot.org/c/coreboot/+/57271
Tested-by: build bot (Jenkins) 
Reviewed-by: Jakub Czapiga 
---
M src/commonlib/bsd/cbfs_private.c
1 file changed, 7 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Jakub Czapiga: Looks good to me, approved

diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c
index 16bab7c..1642cca2 100644
--- a/src/commonlib/bsd/cbfs_private.c
+++ b/src/commonlib/bsd/cbfs_private.c
@@ -3,9 +3,9 @@
 #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)
+static cb_err_t read_next_header(cbfs_dev_t dev, size_t *offset, struct cbfs_file *buffer,
+				 const size_t devsize)
 {
-	const size_t devsize = cbfs_dev_size(dev);
    DEBUG("Looking for next file @%#zx...\n", *offset);
    *offset = ALIGN_UP(*offset, CBFS_ALIGNMENT);
    while (*offset + sizeof(*buffer) < devsize) {
@@ -28,6 +28,7 @@
    	   void *arg, struct vb2_hash *metadata_hash, enum cbfs_walk_flags flags)
 {
    const bool do_hash = CBFS_ENABLE_HASHING && metadata_hash;
+	const size_t devsize = cbfs_dev_size(dev);
    struct vb2_digest_context dc;
    vb2_error_t vbrv;

@@ -41,7 +42,7 @@
    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) {
+	while ((ret_header = read_next_header(dev, &offset, &mdata.h, devsize)) == 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);
@@ -50,8 +51,9 @@

DEBUG("Found CBFS header @%#zx (type %d, attr +%#x, data +%#x, length %#x)\n",
    	      offset, type, attr_offset, data_offset, data_length);
-		if (data_offset > sizeof(mdata)) {
-			ERROR("File metadata @%#zx too large\n", offset);
+		if (data_offset > sizeof(mdata) || data_length > devsize ||
+		    offset + data_offset + data_length > devsize) {
+			ERROR("File @%#zx too large\n", offset);
    		goto next_file;
    	}