From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1fO2Hh-0003ME-Cs for mharc-grub-devel@gnu.org; Wed, 30 May 2018 10:41:05 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fO2He-0003K9-Cc for grub-devel@gnu.org; Wed, 30 May 2018 10:41:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fO2Ha-00079U-GI for grub-devel@gnu.org; Wed, 30 May 2018 10:41:02 -0400 Received: from boksu.net-space.pl ([185.15.1.105]:43303) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1fO2Ha-00077r-40 for grub-devel@gnu.org; Wed, 30 May 2018 10:40:58 -0400 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1841793AbeE3Oky (ORCPT ); Wed, 30 May 2018 16:40:54 +0200 Date: Wed, 30 May 2018 16:40:54 +0200 From: Daniel Kiper To: Andrew Jeddeloh Cc: Daniel Kiper , grub-devel@gnu.org Subject: Re: [PATCH] loader/i386/linux: calculate the size of the setup header Message-ID: <20180530144054.GL27874@router-fw-old.local.net-space.pl> References: <20180510171133.GK25320@router-fw-old.local.net-space.pl> <20180518115301.GA13069@router-fw-old.local.net-space.pl> 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: Wed, 30 May 2018 14:41:03 -0000 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 > 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 > --- > 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