linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
@ 2019-12-02 17:29 Daniel Kiper
  2019-12-08 22:43 ` Daniel Kiper
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Daniel Kiper @ 2019-12-02 17:29 UTC (permalink / raw)
  To: grub-devel, linux-kernel, x86
  Cc: bp, eric.snowberg, hpa, kanth.ghatraju, konrad.wilk, mingo,
	phcoder, rdunlap, ross.philipson

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
  2019-12-02 17:29 [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation Daniel Kiper
@ 2019-12-08 22:43 ` Daniel Kiper
  2019-12-08 22:47   ` hpa
  2019-12-13 12:57 ` Javier Martinez Canillas
  2019-12-13 18:43 ` Ross Philipson
  2 siblings, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2019-12-08 22:43 UTC (permalink / raw)
  To: grub-devel, linux-kernel, x86
  Cc: bp, eric.snowberg, hpa, kanth.ghatraju, konrad.wilk, mingo,
	phcoder, rdunlap, ross.philipson, krystian.hebel,
	maciej.pijanowski, michal.zygowski, piotr.krol

Hmmm... Nobody cares? Strange...

Adding 3mdeb folks...

Daniel

On Mon, Dec 02, 2019 at 06:29:39PM +0100, Daniel Kiper wrote:
> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
  2019-12-08 22:43 ` Daniel Kiper
@ 2019-12-08 22:47   ` hpa
  0 siblings, 0 replies; 5+ messages in thread
From: hpa @ 2019-12-08 22:47 UTC (permalink / raw)
  To: Daniel Kiper, grub-devel, linux-kernel, x86
  Cc: bp, eric.snowberg, kanth.ghatraju, konrad.wilk, mingo, phcoder,
	rdunlap, ross.philipson, krystian.hebel, maciej.pijanowski,
	michal.zygowski, piotr.krol

On December 8, 2019 2:43:57 PM PST, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>Hmmm... Nobody cares? Strange...
>
>Adding 3mdeb folks...
>
>Daniel
>
>On Mon, Dec 02, 2019 at 06:29:39PM +0100, Daniel Kiper wrote:
>> 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

Please fix these problems. It is a huge headache for us in the kennel community.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
  2019-12-02 17:29 [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation Daniel Kiper
  2019-12-08 22:43 ` Daniel Kiper
@ 2019-12-13 12:57 ` Javier Martinez Canillas
  2019-12-13 18:43 ` Ross Philipson
  2 siblings, 0 replies; 5+ messages in thread
From: Javier Martinez Canillas @ 2019-12-13 12:57 UTC (permalink / raw)
  To: The development of GNU GRUB, Daniel Kiper, linux-kernel, x86
  Cc: bp, eric.snowberg, hpa, kanth.ghatraju, konrad.wilk, mingo,
	phcoder, rdunlap, ross.philipson

Hello Daniel,

On 12/2/19 6:29 PM, Daniel Kiper wrote:
> 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.
>

Maybe you should add the following tag?

Fixes: e683cfb0cf5 ("loader/i386/linux: Calculate the setup_header length")

> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
  2019-12-02 17:29 [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation Daniel Kiper
  2019-12-08 22:43 ` Daniel Kiper
  2019-12-13 12:57 ` Javier Martinez Canillas
@ 2019-12-13 18:43 ` Ross Philipson
  2 siblings, 0 replies; 5+ messages in thread
From: Ross Philipson @ 2019-12-13 18:43 UTC (permalink / raw)
  To: Daniel Kiper, grub-devel, linux-kernel, x86
  Cc: bp, eric.snowberg, hpa, kanth.ghatraju, konrad.wilk, mingo,
	phcoder, rdunlap

On 12/02/2019 12:29 PM, Daniel Kiper wrote:
> 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>

Yes this looks correct. The length should be based off the jmp offset
byte and not the size of the structure defined in GRUB.

Reviewed-by: Ross Philipson <ross.philipson@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,
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-12-13 20:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 17:29 [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation Daniel Kiper
2019-12-08 22:43 ` Daniel Kiper
2019-12-08 22:47   ` hpa
2019-12-13 12:57 ` Javier Martinez Canillas
2019-12-13 18:43 ` Ross Philipson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).