Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Daniel Kiper <daniel.kiper@oracle.com>
To:     grub-devel@gnu.org, linux-kernel@vger.kernel.org, x86@kernel.org
Cc:     bp@alien8.de, eric.snowberg@oracle.com, hpa@zytor.com,
        kanth.ghatraju@oracle.com, konrad.wilk@oracle.com,
        mingo@redhat.com, phcoder@gmail.com, rdunlap@infradead.org,
        ross.philipson@oracle.com
Subject: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
Date:   Mon,  2 Dec 2019 18:29:39 +0100
Message-Id: <20191202172939.29271-1-daniel.kiper@oracle.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Recent work around x86 Linux kernel loader revealed an underflow in the
setup_header length calculation and another related issue. Both lead to
the memory overwrite and later machine crash.

Currently when the GRUB copies the setup_header into the linux_params
(struct boot_params, traditionally known as "zero page") it assumes the
setup_header size as sizeof(linux_i386_kernel_header/lh). This is
incorrect. It should use the value calculated accordingly to the Linux
kernel boot protocol. Otherwise in case of pretty old kernel, to be
exact Linux kernel boot protocol, the GRUB may write more into
linux_params than it was expected to. Fortunately this is not very big
issue. Though it has to be fixed. However, there is also an underflow
which is grave. It happens when

  sizeof(linux_i386_kernel_header/lh) > "real size of the setup_header".

Then len value wraps around and grub_file_read() reads whole kernel into
the linux_params overwriting memory past it. This leads to the GRUB
memory allocator breakage and finally to its crash during boot.

The patch fixes both issues. Additionally, it moves the code not related to
grub_memset(linux_params)/grub_memcpy(linux_params)/grub_file_read(linux_params)
section outside of it to not confuse the reader.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/loader/i386/linux.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index d0501e229..ee95cd374 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -761,17 +761,12 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
     goto fail;
 
   grub_memset (&linux_params, 0, sizeof (linux_params));
-  grub_memcpy (&linux_params.setup_sects, &lh.setup_sects, sizeof (lh) - 0x1F1);
-
-  linux_params.code32_start = prot_mode_target + lh.code32_start - GRUB_LINUX_BZIMAGE_ADDR;
-  linux_params.kernel_alignment = (1 << align);
-  linux_params.ps_mouse = linux_params.padding10 =  0;
 
   /*
    * The Linux 32-bit boot protocol defines the setup header end
    * to be at 0x202 + the byte value at 0x201.
    */
-  len = 0x202 + *((char *) &linux_params.jump + 1);
+  len = 0x202 + *((char *) &lh.jump + 1);
 
   /* Verify the struct is big enough so we do not write past the end. */
   if (len > (char *) &linux_params.edd_mbr_sig_buffer - (char *) &linux_params) {
@@ -779,10 +774,13 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
     goto fail;
   }
 
+  grub_memcpy (&linux_params.setup_sects, &lh.setup_sects, len - 0x1F1);
+
   /* 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 ((len > 0) &&
+      (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != len))
     {
       if (!grub_errno)
 	grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
@@ -790,6 +788,9 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       goto fail;
     }
 
+  linux_params.code32_start = prot_mode_target + lh.code32_start - GRUB_LINUX_BZIMAGE_ADDR;
+  linux_params.kernel_alignment = (1 << align);
+  linux_params.ps_mouse = linux_params.padding10 = 0;
   linux_params.type_of_loader = GRUB_LINUX_BOOT_LOADER_TYPE;
 
   /* These two are used (instead of cmd_line_ptr) by older versions of Linux,
-- 
2.11.0