From 195f61375aeec9eec16604ec59f6eda2e6058cc1 Mon Sep 17 00:00:00 2001 From: "Luke T. Shumaker" 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