The gunzip() function reads various fields from a passed in source
buffer in order to skip a header before passing the actual compressed
data to the zlib inflate() function. It does check whether the
passed in buffer is too small, but unfortunately it checks that only
after reading bytes from the src buffer, so it could read off the end
of the buffer.
You can see this with valgrind:
$ printf "%b" '\x1f\x8b' > /tmp/image
$ valgrind qemu-system-aarch64 -display none -M virt -cpu max -kernel /tmp/image
[...]
==19224== Invalid read of size 1
==19224== at 0x67302E: gunzip (loader.c:558)
==19224== by 0x673907: load_image_gzipped_buffer (loader.c:788)
==19224== by 0xA18032: load_aarch64_image (boot.c:932)
==19224== by 0xA18489: arm_setup_direct_kernel_boot (boot.c:1063)
==19224== by 0xA18D90: arm_load_kernel (boot.c:1317)
==19224== by 0x9F3651: machvirt_init (virt.c:2114)
==19224== by 0x794B7A: machine_run_board_init (machine.c:1272)
==19224== by 0xD5CAD3: qemu_init_board (vl.c:2618)
==19224== by 0xD5CCA6: qmp_x_exit_preconfig (vl.c:2692)
==19224== by 0xD5F32E: qemu_init (vl.c:3713)
==19224== by 0x5ADDB1: main (main.c:49)
==19224== Address 0x3802a873 is 0 bytes after a block of size 3 alloc'd
==19224== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19224== by 0x61E7657: g_file_get_contents (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.5600.4)
==19224== by 0x673895: load_image_gzipped_buffer (loader.c:771)
==19224== by 0xA18032: load_aarch64_image (boot.c:932)
==19224== by 0xA18489: arm_setup_direct_kernel_boot (boot.c:1063)
==19224== by 0xA18D90: arm_load_kernel (boot.c:1317)
==19224== by 0x9F3651: machvirt_init (virt.c:2114)
==19224== by 0x794B7A: machine_run_board_init (machine.c:1272)
==19224== by 0xD5CAD3: qemu_init_board (vl.c:2618)
==19224== by 0xD5CCA6: qmp_x_exit_preconfig (vl.c:2692)
==19224== by 0xD5F32E: qemu_init (vl.c:3713)
==19224== by 0x5ADDB1: main (main.c:49)
Check that we have enough bytes of data to read the header bytes that
we read before we read them.
Fixes: Coverity 1458997
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/core/loader.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 5b34869a541..c623318b737 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -555,24 +555,35 @@ ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen)
/* skip header */
i = 10;
+ if (srclen < 4) {
+ goto toosmall;
+ }
flags = src[3];
if (src[2] != DEFLATED || (flags & RESERVED) != 0) {
puts ("Error: Bad gzipped data\n");
return -1;
}
- if ((flags & EXTRA_FIELD) != 0)
+ if ((flags & EXTRA_FIELD) != 0) {
+ if (srclen < 12) {
+ goto toosmall;
+ }
i = 12 + src[10] + (src[11] << 8);
- if ((flags & ORIG_NAME) != 0)
- while (src[i++] != 0)
- ;
- if ((flags & COMMENT) != 0)
- while (src[i++] != 0)
- ;
- if ((flags & HEAD_CRC) != 0)
+ }
+ if ((flags & ORIG_NAME) != 0) {
+ while (i < srclen && src[i++] != 0) {
+ /* do nothing */
+ }
+ }
+ if ((flags & COMMENT) != 0) {
+ while (i < srclen && src[i++] != 0) {
+ /* do nothing */
+ }
+ }
+ if ((flags & HEAD_CRC) != 0) {
i += 2;
+ }
if (i >= srclen) {
- puts ("Error: gunzip out of data in header\n");
- return -1;
+ goto toosmall;
}
s.zalloc = zalloc;
@@ -596,6 +607,10 @@ ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen)
inflateEnd(&s);
return dstbytes;
+
+toosmall:
+ puts("Error: gunzip out of data in header\n");
+ return -1;
}
/* Load a U-Boot image. */
--
2.20.1
On 8/12/21 4:18 PM, Peter Maydell wrote: > The gunzip() function reads various fields from a passed in source > buffer in order to skip a header before passing the actual compressed > data to the zlib inflate() function. It does check whether the > passed in buffer is too small, but unfortunately it checks that only > after reading bytes from the src buffer, so it could read off the end > of the buffer. > > You can see this with valgrind: > > $ printf "%b" '\x1f\x8b' > /tmp/image > $ valgrind qemu-system-aarch64 -display none -M virt -cpu max -kernel /tmp/image Nice :) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > [...] > ==19224== Invalid read of size 1 > ==19224== at 0x67302E: gunzip (loader.c:558) > ==19224== by 0x673907: load_image_gzipped_buffer (loader.c:788) > ==19224== by 0xA18032: load_aarch64_image (boot.c:932) > ==19224== by 0xA18489: arm_setup_direct_kernel_boot (boot.c:1063) > ==19224== by 0xA18D90: arm_load_kernel (boot.c:1317) > ==19224== by 0x9F3651: machvirt_init (virt.c:2114) > ==19224== by 0x794B7A: machine_run_board_init (machine.c:1272) > ==19224== by 0xD5CAD3: qemu_init_board (vl.c:2618) > ==19224== by 0xD5CCA6: qmp_x_exit_preconfig (vl.c:2692) > ==19224== by 0xD5F32E: qemu_init (vl.c:3713) > ==19224== by 0x5ADDB1: main (main.c:49) > ==19224== Address 0x3802a873 is 0 bytes after a block of size 3 alloc'd > ==19224== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==19224== by 0x61E7657: g_file_get_contents (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.5600.4) > ==19224== by 0x673895: load_image_gzipped_buffer (loader.c:771) > ==19224== by 0xA18032: load_aarch64_image (boot.c:932) > ==19224== by 0xA18489: arm_setup_direct_kernel_boot (boot.c:1063) > ==19224== by 0xA18D90: arm_load_kernel (boot.c:1317) > ==19224== by 0x9F3651: machvirt_init (virt.c:2114) > ==19224== by 0x794B7A: machine_run_board_init (machine.c:1272) > ==19224== by 0xD5CAD3: qemu_init_board (vl.c:2618) > ==19224== by 0xD5CCA6: qmp_x_exit_preconfig (vl.c:2692) > ==19224== by 0xD5F32E: qemu_init (vl.c:3713) > ==19224== by 0x5ADDB1: main (main.c:49) > > Check that we have enough bytes of data to read the header bytes that > we read before we read them. > > Fixes: Coverity 1458997 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/core/loader.c | 35 +++++++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 10 deletions(-)
© 2016 - 2024 Red Hat, Inc.