From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1fGp6V-0004I9-NP for mharc-grub-devel@gnu.org; Thu, 10 May 2018 13:11:43 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fGp6T-0004I0-5G for grub-devel@gnu.org; Thu, 10 May 2018 13:11:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fGp6P-0005F0-4n for grub-devel@gnu.org; Thu, 10 May 2018 13:11:41 -0400 Received: from boksu.net-space.pl ([185.15.1.105]:47603) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1fGp6O-0005EB-PG for grub-devel@gnu.org; Thu, 10 May 2018 13:11:37 -0400 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1839017AbeEJRLd (ORCPT ); Thu, 10 May 2018 19:11:33 +0200 Date: Thu, 10 May 2018 19:11:33 +0200 From: Daniel Kiper To: The development of GNU GRUB Cc: andrew.jeddeloh@redhat.com, dkiper@net-space.pl Subject: Re: [PATCH] loader/i386/linux: calculate the size of the setup header Message-ID: <20180510171133.GK25320@router-fw-old.local.net-space.pl> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.3.28i X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 185.15.1.105 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 17:11:42 -0000 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 > --- > 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