Re: [PATCH] loader/i386/linux: calculate the size of the setup header

[Daniel Kiper <dkiper@net-space.pl>]

On Wed, May 09, 2018 at 10:46:47AM -0700, Andrew Jeddeloh wrote:
> This patch is prompted from a question I asked a while ago about why
> the disk read is necessary. See the thread here [1].
>
> This changes the disk read to use the length of the setup header as
> calculated by the x86 32 bit linux boot protocol [1]. I'm not 100%
> sure its patch that's wanted however. The idea was that grub should

There is no doubt. We want this patch.

> only read the amount specified by the boot protocol and not more, but
> it turns out the size of the linux_kernel_params struct is already
> sized such that grub reads the exact amount anyway (at least with the
> recent kernels I've tested with). This introduces two changes:

OK but earlier you have told that linux_kernel_params.e820_map[] is
overwritten. Is it still valid?

>  - if a new version of linux makes the setup headers section larger,
> this will fail instead of only readiing the old fields. I'm not sure
> if this behavior is desired.

It is but math is wrong. Please look below.

>  - If older versions have a smaller setup header section, less will be read.

Exactly.

> [1] http://lists.gnu.org/archive/html/grub-devel/2018-04/msg00073.html
> [2] https://raw.githubusercontent.com/torvalds/linux/master/Documentation/x86/boot.txt
>
>
> Previously the length was just assumed to be the size of the
> linux_params struct. The linux x86 32 bit boot protocol says the size of
> the setup header is 0x202 + the byte at 0x201 in the boot params.
> Calculate the size of the header, rather than assume it is the size of
> the linux_params struct.
>
> Signed-off-by: Andrew Jeddeloh <andrew.jeddeloh@coreos.com>
> ---
>  grub-core/loader/i386/linux.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 44301e126..9b4d33785 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -805,7 +805,16 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
>    linux_params.kernel_alignment = (1 << align);
>    linux_params.ps_mouse = linux_params.padding10 =  0;
>
> -  len = sizeof (linux_params) - sizeof (lh);
> +  // The linux 32 bit boot protocol defines the setup header size to be 0x202 + the size of
> +  // the jump at 0x200. We've already read sizeof(lh)

s/the size of the jump at 0x200/byte value at offset 0x201/

s/We've already read sizeof(lh)//

And please use /* ... */ for comments instead of //

> +  len = *((char *)&linux_params.jump + 1) + 0x202 - sizeof(lh);

len = *((char *)&linux_params.jump + 1) + 0x202;

> +  // Verify the struct is big enough so we do not write past the end
> +  if (len + sizeof(lh) > sizeof(linux_params)) {

if (len > &linux_params.e820_map - &linux_params)

> +    grub_error (GRUB_ERR_BAD_OS, "boot params setup header too big
> for linux_params struct");

s/boot params/Linux/

> +    goto fail;
> +  }

/* We've already read lh so there is not need to read it second time. */
len -= sizeof (lh);

> +
>    if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len)
>      {
>        if (!grub_errno)

I hope that helps.

Daniel