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 <andrew.jedde...@coreos.com>
>>>
>>> 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 <andrew.jedde...@coreos.com>
>>> Signed-off-by: Daniel Kiper <daniel.ki...@oracle.com>
>>> ---
>>> 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 <ross.philip...@oracle.com>
>
> Daniel
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
