Re: [PATCH] loader/i386/linux: Calculate the setup_header length

[Daniel Kiper]

On Fri, Mar 29, 2019 at 11:55:12AM -0400, Ross Philipson wrote:
> On 03/29/2019 11:09 AM, Daniel Kiper wrote:
> > From: Andrew Jeddeloh <address@hidden>
> >
> > Previously the setup_header length was just assumed to be the size of the
> > linux_kernel_params struct. The linux x86 32-bit boot protocol says that
> > the size of the linux_i386_kernel_header is 0x202 + the byte value at 0x201
> > in the linux_i386_kernel_header. Calculate the size of the header, rather
> > than assume it is the size of the linux_kernel_params struct.
> >
> > Additionally, add some required members to the linux_kernel_params
> > struct and align the content of linux_i386_kernel_header struct with
> > it. New members naming was taken directly from Linux kernel source.
> >
> > linux_kernel_params and linux_i386_kernel_header structs require more
> > cleanup. However, this is not urgent, so, let's do this after release.
> > Just in case...
> >
> > Signed-off-by: Andrew Jeddeloh <address@hidden>
> > Signed-off-by: Daniel Kiper <address@hidden>
> > ---
> >  grub-core/loader/i386/linux.c | 16 +++++++++++++++-
> >  include/grub/i386/linux.h     | 14 ++++++++++++--
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> > index b6015913b..73e15a90f 100644
> > --- a/grub-core/loader/i386/linux.c
> > +++ b/grub-core/loader/i386/linux.c
> > @@ -767,7 +767,21 @@ 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 = 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.edd_mbr_sig_buffer - (char *) 
> > &linux_params) {
> > +    grub_error (GRUB_ERR_BAD_OS, "Linux setup header too big");
> > +    goto fail;
> > +  }
> > +
> > +  /* We've already read lh so there is no need to read it second time. */
> > +  len -= sizeof(lh);
> > +
>
> I am a little confused about the logic here and what exactly you are
> trying to get the size of. The size of just the setup_header portion
> would start at 0x1f1 so the logic for finding that would be something like:
>
> len = (0x202 - 0x1f1) + *((char *) &linux_params.jump + 1);
>
> If you are trying to find the size of all the boot params, your
> calculation would leave out edd_mbr_sig_buffer and e820_map which are
> after the end of the setup_header.

Documentation/x86/boot.txt, 32-bit BOOT PROTOCOL section says:

  In 32-bit boot protocol, the first step in loading a Linux kernel
  should be to setup the boot parameters (struct boot_params,
  traditionally known as "zero page"). The memory for struct boot_params
  should be allocated and initialized to all zero. Then the setup header
  from offset 0x01f1 of kernel image on should be loaded into struct
  boot_params and examined. The end of setup header can be calculated as
  follow:

          0x0202 + byte value at offset 0x0201

The problem with currently existing GRUB code is that it reads data into
linux_params past the end of lh. So, we have to properly calculate the
end of lh using algorithm described above. Maybe the comment "The Linux
32-bit boot protocol defines the setup header size to be 0x202 + the
byte value at 0x201." is a bit confusing and I should do s/size/end/

> Note I am using setup_header as a reference to the struct as defined in
> linux. GRUB does not separate it out as it's own struct but just puts
> the fields in the overall linux_kernel_params struct. Maybe it should be
> broken out as a separate structure for clarity.

In general both Linux boot structs are defined in the GRUB in very
confusing way. So, I would suggest to be more tightly tied to the GRUB
code and definitions than relevant code and definitions in the kernel.
Of course the final logic has to be the same and I think that this patch
makes it to happen.

> >    if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != 
> > len)
> >      {
> >        if (!grub_errno)
> > diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
> > index a96059311..ce30e7fb0 100644
> > --- a/include/grub/i386/linux.h
> > +++ b/include/grub/i386/linux.h
> > @@ -46,6 +46,9 @@
> >  #define VIDEO_CAPABILITY_SKIP_QUIRKS       (1 << 0)
> >  #define VIDEO_CAPABILITY_64BIT_BASE        (1 << 1)        /* Frame buffer 
> > base is 64-bit. */
> >
> > +/* Maximum number of MBR signatures to store. */
> > +#define EDD_MBR_SIG_MAX                    16
> > +
> >  #ifdef __x86_64__
> >
> >  #define GRUB_LINUX_EFI_SIGNATURE   \
> > @@ -142,6 +145,7 @@ struct linux_i386_kernel_header
> >    grub_uint64_t setup_data;
> >    grub_uint64_t pref_address;
> >    grub_uint32_t init_size;
> > +  grub_uint32_t handover_offset;
> >  } GRUB_PACKED;
> >
> >  /* Boot parameters for Linux based on 2.6.12. This is used by the setup
> > @@ -273,6 +277,7 @@ struct linux_kernel_params
> >
> >    grub_uint8_t padding9[0x1f1 - 0x1e9];
> >
> > +  /* Linux setup header copy - BEGIN. */
> >    grub_uint8_t setup_sects;                /* The size of the setup in 
> > sectors */
> >    grub_uint16_t root_flags;                /* If the root is mounted 
> > readonly */
> >    grub_uint16_t syssize;           /* obsolete */
> > @@ -311,9 +316,14 @@ struct linux_kernel_params
> >    grub_uint32_t payload_offset;
> >    grub_uint32_t payload_length;
> >    grub_uint64_t setup_data;
> > -  grub_uint8_t pad2[120];          /* 258 */
> > +  grub_uint64_t pref_address;
> > +  grub_uint32_t init_size;
> > +  grub_uint32_t handover_offset;
>
> Yea I noticed these were missing, good to add them. You should move that
> comment down nd update it to /* 268 */

Yeah, good point.

> Also since you added these here, do you want to add them to
> linux_i386_kernel_header for consistency? That struct is still stuck at
> boot protocol verion 2.10 as stated in the comment.

linux_i386_kernel_header is missing handover_offset only. So,
I added it in this patch too. It is a bit above.

Anyway, I am aware that linux_i386_kernel_header and linux_kernel_params
are defined in very confusing way. They both have to be fixed. However,
I am not going to do this just before the release. I do not want to
break something by mistake. So, I am just only fixing the grave issue.

Daniel