* [PATCH] loader/i386/linux: calculate the size of the setup header @ 2018-05-09 17:46 Andrew Jeddeloh 2018-05-10 17:11 ` Daniel Kiper 0 siblings, 1 reply; 5+ messages in thread From: Andrew Jeddeloh @ 2018-05-09 17:46 UTC (permalink / raw) To: grub-devel This patch is prompted from a question I asked a while ago about why the disk read is necessary. See the thread here [1]. This changes the disk read to use the length of the setup header as calculated by the x86 32 bit linux boot protocol [1]. I'm not 100% sure its patch that's wanted however. The idea was that grub should only read the amount specified by the boot protocol and not more, but it turns out the size of the linux_kernel_params struct is already sized such that grub reads the exact amount anyway (at least with the recent kernels I've tested with). This introduces two changes: - if a new version of linux makes the setup headers section larger, this will fail instead of only readiing the old fields. I'm not sure if this behavior is desired. - If older versions have a smaller setup header section, less will be read. [1] http://lists.gnu.org/archive/html/grub-devel/2018-04/msg00073.html [2] https://raw.githubusercontent.com/torvalds/linux/master/Documentation/x86/boot.txt 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.jeddeloh@coreos.com> --- grub-core/loader/i386/linux.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c index 44301e126..9b4d33785 100644 --- a/grub-core/loader/i386/linux.c +++ b/grub-core/loader/i386/linux.c @@ -805,7 +805,16 @@ 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 size of + // the jump at 0x200. We've already read sizeof(lh) + len = *((char *)&linux_params.jump + 1) + 0x202 - sizeof(lh); + + // Verify the struct is big enough so we do not write past the end + if (len + sizeof(lh) > sizeof(linux_params)) { + grub_error (GRUB_ERR_BAD_OS, "boot params setup header too big for linux_params struct"); + goto fail; + } + if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len) { if (!grub_errno) -- 2.14.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] loader/i386/linux: calculate the size of the setup header 2018-05-09 17:46 [PATCH] loader/i386/linux: calculate the size of the setup header Andrew Jeddeloh @ 2018-05-10 17:11 ` Daniel Kiper [not found] ` <CAK=WzwcAa5kjyT+Tupr5jedX+XiW1i4hDjknLZHEst1EGLOmtg@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Daniel Kiper @ 2018-05-10 17:11 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: andrew.jeddeloh, dkiper On Wed, May 09, 2018 at 10:46:47AM -0700, Andrew Jeddeloh wrote: > This patch is prompted from a question I asked a while ago about why > the disk read is necessary. See the thread here [1]. > > This changes the disk read to use the length of the setup header as > calculated by the x86 32 bit linux boot protocol [1]. I'm not 100% > sure its patch that's wanted however. The idea was that grub should There is no doubt. We want this patch. > only read the amount specified by the boot protocol and not more, but > it turns out the size of the linux_kernel_params struct is already > sized such that grub reads the exact amount anyway (at least with the > recent kernels I've tested with). This introduces two changes: OK but earlier you have told that linux_kernel_params.e820_map[] is overwritten. Is it still valid? > - if a new version of linux makes the setup headers section larger, > this will fail instead of only readiing the old fields. I'm not sure > if this behavior is desired. It is but math is wrong. Please look below. > - If older versions have a smaller setup header section, less will be read. Exactly. > [1] http://lists.gnu.org/archive/html/grub-devel/2018-04/msg00073.html > [2] https://raw.githubusercontent.com/torvalds/linux/master/Documentation/x86/boot.txt > > > 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.jeddeloh@coreos.com> > --- > grub-core/loader/i386/linux.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c > index 44301e126..9b4d33785 100644 > --- a/grub-core/loader/i386/linux.c > +++ b/grub-core/loader/i386/linux.c > @@ -805,7 +805,16 @@ 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 size of > + // the jump at 0x200. We've already read sizeof(lh) s/the size of the jump at 0x200/byte value at offset 0x201/ s/We've already read sizeof(lh)// And please use /* ... */ for comments instead of // > + len = *((char *)&linux_params.jump + 1) + 0x202 - sizeof(lh); len = *((char *)&linux_params.jump + 1) + 0x202; > + // Verify the struct is big enough so we do not write past the end > + if (len + sizeof(lh) > sizeof(linux_params)) { if (len > &linux_params.e820_map - &linux_params) > + grub_error (GRUB_ERR_BAD_OS, "boot params setup header too big > for linux_params struct"); s/boot params/Linux/ > + goto fail; > + } /* We've already read lh so there is not need to read it second time. */ len -= sizeof (lh); > + > if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len) > { > if (!grub_errno) I hope that helps. Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAK=WzwcAa5kjyT+Tupr5jedX+XiW1i4hDjknLZHEst1EGLOmtg@mail.gmail.com>]
* Re: [PATCH] loader/i386/linux: calculate the size of the setup header [not found] ` <CAK=WzwcAa5kjyT+Tupr5jedX+XiW1i4hDjknLZHEst1EGLOmtg@mail.gmail.com> @ 2018-05-18 11:53 ` Daniel Kiper 2018-05-25 19:02 ` Andrew Jeddeloh 0 siblings, 1 reply; 5+ messages in thread From: Daniel Kiper @ 2018-05-18 11:53 UTC (permalink / raw) To: Andrew Jeddeloh; +Cc: Daniel Kiper, grub-devel Re-added grub-devel. Next time please do not drop GRUB ML from the addresses. On Thu, May 17, 2018 at 03:40:32PM -0700, Andrew Jeddeloh wrote: > Sorry about the long delay, I agree with all the sugguestions, except shouldn't No problem. > if (len > &linux_params.e820_map - &linux_params) > > be > > if (len > sizeof(linux_params)) > > since at that point len is the total size of the header which > linux_params represents? Please take a look at arch/x86/include/uapi/asm/bootparam.h in latest Linux kernel source. My proposal is better but it seems to me right now that it is still too much. I have a feeling that we should not go beyond the end of boot_params._pad7. Am I right? Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] loader/i386/linux: calculate the size of the setup header 2018-05-18 11:53 ` Daniel Kiper @ 2018-05-25 19:02 ` Andrew Jeddeloh 2018-05-30 14:40 ` Daniel Kiper 0 siblings, 1 reply; 5+ messages in thread From: Andrew Jeddeloh @ 2018-05-25 19:02 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel Oops, my bad, didn't mean to drop the ML. 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 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 corresponding field so e820_map seems good enough. Thanks for working through this. Here's the revised patch From d46c75b4a9151c10e7f7809637f9f42a2c393c25 Mon Sep 17 00:00:00 2001 From: Andrew Jeddeloh <andrew.jeddeloh@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.jeddeloh@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; + + /* Verify the struct is big enough so we do not write past the end */ + if (len > (char *)&linux_params.e820_map - (char *)&linux_params) { + 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) -- 2.14.1 On Fri, May 18, 2018 at 4:53 AM, Daniel Kiper <dkiper@net-space.pl> wrote: > Re-added grub-devel. Next time please do not drop GRUB ML from the addresses. > > On Thu, May 17, 2018 at 03:40:32PM -0700, Andrew Jeddeloh wrote: >> Sorry about the long delay, I agree with all the sugguestions, except shouldn't > > No problem. > >> if (len > &linux_params.e820_map - &linux_params) >> >> be >> >> if (len > sizeof(linux_params)) >> >> since at that point len is the total size of the header which >> linux_params represents? > > Please take a look at arch/x86/include/uapi/asm/bootparam.h in latest > Linux kernel source. My proposal is better but it seems to me right now > that it is still too much. I have a feeling that we should not go beyond > the end of boot_params._pad7. Am I right? > > Daniel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] loader/i386/linux: calculate the size of the setup header 2018-05-25 19:02 ` Andrew Jeddeloh @ 2018-05-30 14:40 ` Daniel Kiper 0 siblings, 0 replies; 5+ messages in thread From: Daniel Kiper @ 2018-05-30 14:40 UTC (permalink / raw) To: Andrew Jeddeloh; +Cc: Daniel Kiper, grub-devel 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.jeddeloh@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.jeddeloh@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-30 14:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-09 17:46 [PATCH] loader/i386/linux: calculate the size of the setup header Andrew Jeddeloh 2018-05-10 17:11 ` Daniel Kiper [not found] ` <CAK=WzwcAa5kjyT+Tupr5jedX+XiW1i4hDjknLZHEst1EGLOmtg@mail.gmail.com> 2018-05-18 11:53 ` Daniel Kiper 2018-05-25 19:02 ` Andrew Jeddeloh 2018-05-30 14:40 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.