Merge pull request 'Fix building vboot on i686' (#218) from lukeshu/lbmk:lukeshu/i686 into master

Reviewed-on: https://codeberg.org/libreboot/lbmk/pulls/218

2 files changed, 178 insertions, 35 deletions

diff --git a/config/submodule/coreboot/dell/vboot/patches/0001-don-t-treat-warnings-as-errors.patch b/config/submodule/coreboot/dell/vboot/patches/0001-don-t-treat-warnings-as-errors.patch
deleted file mode 100644
index 9e14dc7d..00000000
--- a/config/submodule/coreboot/dell/vboot/patches/0001-don-t-treat-warnings-as-errors.patch
+++ /dev/null
@@ -1,35 +0,0 @@
-From d94300a671688746f2fb3d77eefa631a3ed90306 Mon Sep 17 00:00:00 2001
-From: Leah Rowe <info@minifree.org>
-Date: Sun, 19 May 2024 23:35:52 +0100
-Subject: [PATCH 1/1] don't treat warnings as errors
-
-Signed-off-by: Leah Rowe <info@minifree.org>
----
- Makefile | 4 ++--
- 1 file changed, 2 insertions(+), 2 deletions(-)
-
-diff --git a/Makefile b/Makefile
-index 4cb265b2..4ba8b2da 100644
---- a/Makefile
-+++ b/Makefile
-@@ -114,7 +114,7 @@ endif
- # Provide default CC and CFLAGS for firmware builds; if you have any -D flags,
- # please add them after this point (e.g., -DVBOOT_DEBUG).
- DEBUG_FLAGS := $(if $(filter-out 0,${DEBUG}),-g -Og,-g -Os)
--WERROR := -Werror
-+WERROR := -Wno-error -w
- FIRMWARE_FLAGS := -nostdinc -ffreestanding -fno-builtin -fno-stack-protector
- COMMON_FLAGS := -pipe ${WERROR} -Wall -Wstrict-prototypes -Wtype-limits \
- 	-Wundef -Wmissing-prototypes -Wno-trigraphs -Wredundant-decls -Wshadow \
-@@ -128,7 +128,7 @@ COMMON_FLAGS := -pipe ${WERROR} -Wall -Wstrict-prototypes -Wtype-limits \
- # returns: $(1) if compiler was successful, empty string otherwise
- test_ccflag = $(shell \
- 	printf "$(2)\nvoid _start(void) {}\n" | \
--	$(CC) -nostdlib -Werror $(1) -xc -c - -o /dev/null \
-+	$(CC) -nostdlib -Wno-error -w $(1) -xc -c - -o /dev/null \
- 	>/dev/null 2>&1 && echo "$(1)")
- 
- COMMON_FLAGS += $(call test_ccflag,-Wimplicit-fallthrough)
--- 
-2.39.2
-
diff --git a/config/submodule/coreboot/dell/vboot/patches/0001-extract_vmlinuz.c-Fix-the-bounds-check-on-vmlinuz_he.patch b/config/submodule/coreboot/dell/vboot/patches/0001-extract_vmlinuz.c-Fix-the-bounds-check-on-vmlinuz_he.patch
new file mode 100644
index 00000000..1ac41de6
--- /dev/null
+++ b/config/submodule/coreboot/dell/vboot/patches/0001-extract_vmlinuz.c-Fix-the-bounds-check-on-vmlinuz_he.patch
@@ -0,0 +1,178 @@
+From 195f61375aeec9eec16604ec59f6eda2e6058cc1 Mon Sep 17 00:00:00 2001
+From: "Luke T. Shumaker" <lukeshu@lukeshu.com>
+Date: Thu, 30 May 2024 14:08:33 -0600
+Subject: [PATCH 1/1] extract_vmlinuz.c: Fix the bounds check on
+ vmlinuz_header_{offset,size}
+
+The check on vmlinuz_header_offset and vmlinuz_header_size is obviously
+wrong:
+
+	if (!vmlinuz_header_size ||
+	    kpart_data + vmlinuz_header_offset + vmlinuz_header_size >
+	    kpart_data) {
+		return 1;
+	}
+
+`kpart_data + some_unsigned_values` can obviously never be `> kpart_data`,
+unless something has overflowed!  And `vmlinuz_header_offset` hasn't even
+been set yet (besides being initialized to zero)!
+
+GCC will deduce that if the check didn't cause the function to bail, then
+vmlinuz_header_size (a uint32_t) must be "negative"; that is: in the range
+[2GiB,4GiB).
+
+On platforms where size_t is 32-bits, this is *especially* broken.
+memcpy's size argument must be in the range [0,2GiB).  Because GCC has
+proved that vmlinuz_header_size is higher than that, it will fail to
+compile:
+
+	host/lib/extract_vmlinuz.c:67:9: error: 'memcpy' specified bound between 2147483648 and 4294967295 exceeds maximum object size 2147483647 [-Werror=stringop-overflow=]
+
+So, fix the check.
+
+I can now say that what I suspect the original author meant to write would
+be the following patch, if `vmlinuz_header_offset` were already set:
+
+	-kpart_data + vmlinuz_header_offset + vmlinuz_header_size > kpart_data
+	+now        + vmlinuz_header_offset + vmlinuz_header_size > kpart_size
+
+This hypothesis is supported by `now` not getting incremented by
+`kblob_size` the way it is for the keyblock and preamble sizes.
+
+However, we can also see that even this "corrected" bounds check is
+insufficient: it does not detect the vmlinuz_header overflowing into
+kblob_data.
+
+OK, so let's describe the fix:
+
+Have a `*vmlinuz_header` pointer instead of a
+`uint64_t vmlinuz_header_offset`, to be more similar to all the other
+regions.  With this change, the correct check becomes a simple
+
+      vmlinuz_header + vmlinuz_header_size > kblob_data
+
+While we're at it, make some changes that could have helped avoid this in
+the first place:
+
+ - Add comments.
+ - Calculate the vmlinuz_header offset right away, instead of waiting.
+ - Go ahead and increment `now` by `kblob_size`, to increase regularity.
+
+Change-Id: I5c03e49070b6dd2e04459566ef7dd129d27736e4
+---
+ host/lib/extract_vmlinuz.c | 72 +++++++++++++++++++++++++++-----------
+ 1 file changed, 51 insertions(+), 21 deletions(-)
+
+diff --git a/host/lib/extract_vmlinuz.c b/host/lib/extract_vmlinuz.c
+index 4ccfcf33..d2c09443 100644
+--- a/host/lib/extract_vmlinuz.c
++++ b/host/lib/extract_vmlinuz.c
+@@ -15,16 +15,44 @@
+ 
+ int ExtractVmlinuz(void *kpart_data, size_t kpart_size,
+ 		   void **vmlinuz_out, size_t *vmlinuz_size) {
++	// We're going to be extracting `vmlinuz_header` and
++	// `kblob_data`, and returning the concatenation of them.
++	//
++	// kpart_data = +-[kpart_size]------------------------------------+
++	//              |                                                 |
++	//  keyblock  = | +-[keyblock->keyblock_size]-------------------+ |
++	//              | | struct vb2_keyblock          keyblock       | |
++	//              | | char []                      ...data...     | |
++	//              | +---------------------------------------------+ |
++	//              |                                                 |
++	//  preamble  = | +-[preamble->preamble_size]-------------------+ |
++	//              | | struct vb2_kernel_preamble   preamble       | |
++	//              | | char []                       ...data...    | |
++	//              | | char []                      vmlinuz_header | |
++	//              | | char []                       ...data...    | |
++	//              | +---------------------------------------------+ |
++	//              |                                                 |
++	//  kblob_data= | +-[preamble->body_signature.data_size]--------+ |
++	//              | | char []                       ...data...    | |
++	//              | +---------------------------------------------+ |
++	//              |                                                 |
++	//              +-------------------------------------------------+
++
+ 	size_t now = 0;
++	// The 3 sections of kpart_data.
++	struct vb2_keyblock *keyblock = NULL;
+ 	struct vb2_kernel_preamble *preamble = NULL;
+ 	uint8_t *kblob_data = NULL;
+ 	uint32_t kblob_size = 0;
++	// vmlinuz_header
++	uint8_t *vmlinuz_header = NULL;
+ 	uint32_t vmlinuz_header_size = 0;
+-	uint64_t vmlinuz_header_address = 0;
+-	uint64_t vmlinuz_header_offset = 0;
++	// The concatenated result.
+ 	void *vmlinuz = NULL;
+ 
+-	struct vb2_keyblock *keyblock = (struct vb2_keyblock *)kpart_data;
++	// Isolate the 3 sections of kpart_data.
++
++	keyblock = (struct vb2_keyblock *)kpart_data;
+ 	now += keyblock->keyblock_size;
+ 	if (now > kpart_size)
+ 		return 1;
+@@ -36,37 +64,39 @@ int ExtractVmlinuz(void *kpart_data, size_t kpart_size,
+ 
+ 	kblob_data = kpart_data + now;
+ 	kblob_size = preamble->body_signature.data_size;
+-
+-	if (!kblob_data || (now + kblob_size) > kpart_size)
++	now += kblob_size;
++	if (now > kpart_size)
+ 		return 1;
+ 
++	// Find `vmlinuz_header` within `preamble`.
++
+ 	if (preamble->header_version_minor > 0) {
+-		vmlinuz_header_address = preamble->vmlinuz_header_address;
++		// calculate the vmlinuz_header offset from
++		// the beginning of the kpart_data.  The kblob doesn't
++		// include the body_load_offset, but does include
++		// the keyblock and preamble sections.
++		size_t vmlinuz_header_offset =
++			preamble->vmlinuz_header_address -
++			preamble->body_load_address +
++			keyblock->keyblock_size +
++			preamble->preamble_size;
++
++		vmlinuz_header = kpart_data + vmlinuz_header_offset;
+ 		vmlinuz_header_size = preamble->vmlinuz_header_size;
+ 	}
+ 
+-	if (!vmlinuz_header_size ||
+-	    kpart_data + vmlinuz_header_offset + vmlinuz_header_size >
+-	    kpart_data) {
++	if (!vmlinuz_header ||
++	    !vmlinuz_header_size ||
++	    vmlinuz_header + vmlinuz_header_size > kblob_data) {
+ 		return 1;
+ 	}
+ 
+-	// calculate the vmlinuz_header offset from
+-	// the beginning of the kpart_data.  The kblob doesn't
+-	// include the body_load_offset, but does include
+-	// the keyblock and preamble sections.
+-	vmlinuz_header_offset = vmlinuz_header_address -
+-		preamble->body_load_address +
+-		keyblock->keyblock_size +
+-		preamble->preamble_size;
++	// Concatenate and return.
+ 
+ 	vmlinuz = malloc(vmlinuz_header_size + kblob_size);
+ 	if (vmlinuz == NULL)
+ 		return 1;
+-
+-	memcpy(vmlinuz, kpart_data + vmlinuz_header_offset,
+-	       vmlinuz_header_size);
+-
++	memcpy(vmlinuz, vmlinuz_header, vmlinuz_header_size);
+ 	memcpy(vmlinuz + vmlinuz_header_size, kblob_data, kblob_size);
+ 
+ 	*vmlinuz_out = vmlinuz;
+-- 
+2.45.1
+