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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On December 8, 2019 2:43:57 PM PST, Daniel Kiper <[email protected]> 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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
> ---
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas <[email protected]>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
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 <[email protected]>
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 <[email protected]>
> ---
> 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,
>