Re: [PATCH] hw/core/loader: In gunzip(), check index is in range before use, not after

[Philippe Mathieu-Daudé]

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(-)