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

[Ross Philipson]

On 04/01/2019 07:10 AM, Daniel Kiper wrote:
> 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/

Yes calling it end would make it much clearer and that is what the
snippet of docs you pasted above calls it. Now I understand what you are
calculating.

> 
>> 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.

Fair enough.

> 
>>>    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.

So I think you should change the above from size to end an with that:

Reviewed-by: Ross Philipson <address@hidden>

> 
> Daniel
>