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

[Ross Philipson]

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.

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.

>    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 */

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 setup header copy - END. */
> +
> +  grub_uint8_t _pad7[40];
> +  grub_uint32_t edd_mbr_sig_buffer[EDD_MBR_SIG_MAX]; /* 290 */
>    struct grub_e820_mmap e820_map[(0x400 - 0x2d0) / 20];      /* 2d0 */
> -
>  } GRUB_PACKED;
>  #endif /* ! ASM_FILE */
>  
>