* 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