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