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 <andrew.jedde...@coreos.com>
> 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 <andrew.jedde...@coreos.com>
> ---
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
