Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation

[Javier Martinez Canillas <fmartine@redhat.com>]

Hello Daniel,

On 12/2/19 6:29 PM, Daniel Kiper wrote:
> Recent work around x86 Linux kernel loader revealed an underflow in the
> setup_header length calculation and another related issue. Both lead to
> the memory overwrite and later machine crash.
> 
> Currently when the GRUB copies the setup_header into the linux_params
> (struct boot_params, traditionally known as "zero page") it assumes the
> setup_header size as sizeof(linux_i386_kernel_header/lh). This is
> incorrect. It should use the value calculated accordingly to the Linux
> kernel boot protocol. Otherwise in case of pretty old kernel, to be
> exact Linux kernel boot protocol, the GRUB may write more into
> linux_params than it was expected to. Fortunately this is not very big
> issue. Though it has to be fixed. However, there is also an underflow
> which is grave. It happens when
> 
>   sizeof(linux_i386_kernel_header/lh) > "real size of the setup_header".
> 
> Then len value wraps around and grub_file_read() reads whole kernel into
> the linux_params overwriting memory past it. This leads to the GRUB
> memory allocator breakage and finally to its crash during boot.
> 
> The patch fixes both issues. Additionally, it moves the code not related to
> grub_memset(linux_params)/grub_memcpy(linux_params)/grub_file_read(linux_params)
> section outside of it to not confuse the reader.
>

Maybe you should add the following tag?

Fixes: e683cfb0cf5 ("loader/i386/linux: Calculate the setup_header length")

> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat