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

