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

[Daniel Kiper]

On Fri, May 25, 2018 at 12:02:03PM -0700, Andrew Jeddeloh wrote:
> Oops, my bad, didn't mean to drop the ML.

No problem. Sometimes it happens.

> So you're saying the setup header could expand to include some of pad7
> in the future, but we error if it goes beyond that (into the e820

Yep!

> map)? Looking at bootparam.h it looks like the _most_ correct thing
> would be to stop at edd_mbr_sig_buffer, but grub doesn't have a

Exactly!

> corresponding field so e820_map seems good enough. Thanks for working

I would suggest to add edd_mbr_sig_buffer[] to linux_kernel_params.
It does not cost a lot.

> through this.
>
> Here's the revised patch

Next time please post the patch in separate email.

> >From d46c75b4a9151c10e7f7809637f9f42a2c393c25 Mon Sep 17 00:00:00 2001
> From: Andrew Jeddeloh <address@hidden>
> Date: Tue, 8 May 2018 14:39:01 -0700
> Subject: [PATCH] loader/i386/linux: calculate setup header length
>
> 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 <address@hidden>
> ---
>  grub-core/loader/i386/linux.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 44301e126..b5b65ea36 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -805,7 +805,19 @@ 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 byte value
> +   * at 0x201. */
> +  len = *((char *)&linux_params.jump + 1) + 0x202;

Let's be in line with the comment:
  len = 0x202 + *((char *) &linux_params.jump + 1);

> +  /* Verify the struct is big enough so we do not write past the end */
> +  if (len > (char *)&linux_params.e820_map - (char *)&linux_params) {

Lack of space between "(char *)" and "&".

> +    grub_error (GRUB_ERR_BAD_OS, "boot params setup header too big");
> +    goto fail;
> +  }
> +
> +  /* We've already read lh so there is no need to read it second time. */
> +  len -= sizeof(lh);
> +
>    if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != 
> len)
>      {
>        if (!grub_errno)

Otherwise, +/- edd_mbr_sig_buffer[], the patch LGTM.

Daniel