What is this grub_disk_read doing in the i386 linux loader?

What is this grub_disk_read doing in the i386 linux loader?

[Andrew Jeddeloh]

While solving a bug in the coreos fork of grub I came across this disk
read in the i386 linux loader [1]. It looks like its reading whatever
is after the boot param header in the kernel file (defined by the
linux x86 boot protocol [2]) into the rest of the `linux_params`
struct. In practice this means overwriting part of the padding and the
e820 map. As far as I can tell, this is not necessary or a useful
thing to do. Am I missing something?

The bug we were hitting on our fork was miscalculating
(char*)&linux_params + sizeoh(lh) as &linux_params + sizeof(lh), which
(in addition to corrupting memory) means the contents wasn't being
written to (char*)&linux_params + sizeof(lh). However the machines
seem to boot just fine when the memory corruption didn't cause
problems. If I nop out the call to read that chunk into
(char*)linux_params + sizeof(lh) it also seems to boot fine.

Is this intended? If so what is it doing? It dates back to the
original i386 linux loader support [3], but I can't figure out why
this would be intended.

Thanks,
- Andrew

[1] http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c#n809
[2] https://www.kernel.org/doc/Documentation/x86/boot.txt
[3] http://git.savannah.gnu.org/cgit/grub.git/commit/grub-core/loader/i386/linux.c?id=8c411768822a75c8c15108872191a05e84befa6e

Re: What is this grub_disk_read doing in the i386 linux loader?

[Daniel Kiper]

On Thu, Apr 19, 2018 at 03:22:55PM -0700, Andrew Jeddeloh wrote:
> While solving a bug in the coreos fork of grub I came across this disk
> read in the i386 linux loader [1]. It looks like its reading whatever
> is after the boot param header in the kernel file (defined by the
> linux x86 boot protocol [2]) into the rest of the `linux_params`
> struct. In practice this means overwriting part of the padding and the
> e820 map. As far as I can tell, this is not necessary or a useful
> thing to do. Am I missing something?
>
> The bug we were hitting on our fork was miscalculating
> (char*)&linux_params + sizeoh(lh) as &linux_params + sizeof(lh), which
> (in addition to corrupting memory) means the contents wasn't being
> written to (char*)&linux_params + sizeof(lh). However the machines
> seem to boot just fine when the memory corruption didn't cause
> problems. If I nop out the call to read that chunk into
> (char*)linux_params + sizeof(lh) it also seems to boot fine.
>
> Is this intended? If so what is it doing? It dates back to the
> original i386 linux loader support [3], but I can't figure out why
> this would be intended.

This is intended. Look at [2], 32-bit BOOT PROTOCOL paragraph. However,
at least since commit 2001169 math is wrong. I think that len should be
calculated according to Linux boot protocol spec. The destination for
read should be &linux_params.setup_sects.

May I ask you to provide relevant patch for upstream?

Daniel

[1] http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c#n809
[2] https://www.kernel.org/doc/Documentation/x86/boot.txt
[3] http://git.savannah.gnu.org/cgit/grub.git/commit/grub-core/loader/i386/linux.c?id=8c411768822a75c8c15108872191a05e84befa6e

Re: What is this grub_disk_read doing in the i386 linux loader?

[Andrew Jeddeloh]

Thanks for the reply.

I'm not sure I follow. Looking over the 32 bit boot spec, it looks
like the process is:

1) zero out linux_params
 - grub does this
2) copy the linux boot params (from 0x1f1) into linux params
 - grub does this by reading from 0x0 until the end of lh, then
copying lh+0x1f1 til the end of lh into linux_headers [4][5]
3) Do the read/write/modify operations that would happen for a 16 bit boot
4) calculate the end of the the setup header
 - grub does not do this and I think assumes its just the end of the
linux_params struct
5) fill out the rest of the setup header things from the zero page doc [6]
 - AFAICT this isn't meant to be read from the kernel image, but
instead filled out by bootloader or left as zero
   - It looks like the current code is trying to do this but is just
assuming the end is at the end of linux_params.
 - Looking over the items in the zero page doc, there doesn't seem to
be anything that the kernel could supply a useful "default" value.

As I understand it, the read is not harmful (as it just gets
overwritten by the bootloader later) but also not needed. Is this your
understanding?

I'll definitely submit a patch upstream once I figure out what should
be going on.

Thanks
 - Andrew

[4] http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c#n702
[5] http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c#n802
[6] https://www.kernel.org/doc/Documentation/x86/zero-page.txt

On Tue, Apr 24, 2018 at 5:56 AM, Daniel Kiper <dkiper@net-space.pl> wrote:
> On Thu, Apr 19, 2018 at 03:22:55PM -0700, Andrew Jeddeloh wrote:
>> While solving a bug in the coreos fork of grub I came across this disk
>> read in the i386 linux loader [1]. It looks like its reading whatever
>> is after the boot param header in the kernel file (defined by the
>> linux x86 boot protocol [2]) into the rest of the `linux_params`
>> struct. In practice this means overwriting part of the padding and the
>> e820 map. As far as I can tell, this is not necessary or a useful
>> thing to do. Am I missing something?
>>
>> The bug we were hitting on our fork was miscalculating
>> (char*)&linux_params + sizeoh(lh) as &linux_params + sizeof(lh), which
>> (in addition to corrupting memory) means the contents wasn't being
>> written to (char*)&linux_params + sizeof(lh). However the machines
>> seem to boot just fine when the memory corruption didn't cause
>> problems. If I nop out the call to read that chunk into
>> (char*)linux_params + sizeof(lh) it also seems to boot fine.
>>
>> Is this intended? If so what is it doing? It dates back to the
>> original i386 linux loader support [3], but I can't figure out why
>> this would be intended.
>
> This is intended. Look at [2], 32-bit BOOT PROTOCOL paragraph. However,
> at least since commit 2001169 math is wrong. I think that len should be
> calculated according to Linux boot protocol spec. The destination for
> read should be &linux_params.setup_sects.
>
> May I ask you to provide relevant patch for upstream?
>
> Daniel
>
> [1] http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c#n809
> [2] https://www.kernel.org/doc/Documentation/x86/boot.txt
> [3] http://git.savannah.gnu.org/cgit/grub.git/commit/grub-core/loader/i386/linux.c?id=8c411768822a75c8c15108872191a05e84befa6e

Re: What is this grub_disk_read doing in the i386 linux loader?

[Daniel Kiper]

On Tue, Apr 24, 2018 at 04:08:57PM -0700, Andrew Jeddeloh wrote:
> Thanks for the reply.
>
> I'm not sure I follow. Looking over the 32 bit boot spec, it looks
> like the process is:
>
> 1) zero out linux_params
>  - grub does this
> 2) copy the linux boot params (from 0x1f1) into linux params
>  - grub does this by reading from 0x0 until the end of lh, then
> copying lh+0x1f1 til the end of lh into linux_headers [4][5]
> 3) Do the read/write/modify operations that would happen for a 16 bit boot
> 4) calculate the end of the the setup header
>  - grub does not do this and I think assumes its just the end of the
> linux_params struct

As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.

> 5) fill out the rest of the setup header things from the zero page doc [6]
>  - AFAICT this isn't meant to be read from the kernel image, but
> instead filled out by bootloader or left as zero

AIUI it is. Please look above.

>    - It looks like the current code is trying to do this but is just
> assuming the end is at the end of linux_params.

This is wrong due to grub_e820_mmap.

>  - Looking over the items in the zero page doc, there doesn't seem to
> be anything that the kernel could supply a useful "default" value.

GRUB have to load the rest of Linux header into linux_params.
Current math is wrong and have to be fixed. Please look above
and think about newer versions of Linux headers which GRUB is
not aware of.

> As I understand it, the read is not harmful (as it just gets
> overwritten by the bootloader later) but also not needed. Is this your
> understanding?

This read is needed, however, math has to be fixed. Additionally,
I think that you can take a look at SYSLINUX and do something
similar in GRUB.

> I'll definitely submit a patch upstream once I figure out what should
> be going on.

Great! Thanks!

Daniel

Re: What is this grub_disk_read doing in the i386 linux loader?

[Andrew Jeddeloh]

> As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.

I think Michael's message got lost; I think he replied to just you? I
didn't receive it and I don't see it in the archives.

> AIUI it is. Please look above.

I don't know what you're refering to here by "above".

> This is wrong due to grub_e820_mmap.

My understanding is wrong or grub's behavior is wrong? Can you elaborate please?

> This read is needed

I don't understand why. Again, looking at the zero page documentation,
there's nothing after the 16 bit boot params that the linux image
could know ahead of time. Is your concern that future fields could be
added that do need to be read?

Regardless, we can't calculate then length then blindly use it reading
into a struct. If the length grows larger than the size of the struct
we'll start corrupting memory when doing the read. Assuming the read
is needed, we'll need to malloc the amount that still needs to be read
then probably copy that into a struct to modify (and ignore the bits
we don't understand in the case of a new Linux release adding fields
grub doesn't have yet).

Sorry for all the questions, I want to ensure that I understand what's
supposed to be happening first.

Thanks,
 - Andrew

On Wed, Apr 25, 2018 at 1:59 AM, Daniel Kiper <dkiper@net-space.pl> wrote:
> On Tue, Apr 24, 2018 at 04:08:57PM -0700, Andrew Jeddeloh wrote:
>> Thanks for the reply.
>>
>> I'm not sure I follow. Looking over the 32 bit boot spec, it looks
>> like the process is:
>>
>> 1) zero out linux_params
>>  - grub does this
>> 2) copy the linux boot params (from 0x1f1) into linux params
>>  - grub does this by reading from 0x0 until the end of lh, then
>> copying lh+0x1f1 til the end of lh into linux_headers [4][5]
>> 3) Do the read/write/modify operations that would happen for a 16 bit boot
>> 4) calculate the end of the the setup header
>>  - grub does not do this and I think assumes its just the end of the
>> linux_params struct
>
> As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.
>
>> 5) fill out the rest of the setup header things from the zero page doc [6]
>>  - AFAICT this isn't meant to be read from the kernel image, but
>> instead filled out by bootloader or left as zero
>
> AIUI it is. Please look above.
>
>>    - It looks like the current code is trying to do this but is just
>> assuming the end is at the end of linux_params.
>
> This is wrong due to grub_e820_mmap.
>
>>  - Looking over the items in the zero page doc, there doesn't seem to
>> be anything that the kernel could supply a useful "default" value.
>
> GRUB have to load the rest of Linux header into linux_params.
> Current math is wrong and have to be fixed. Please look above
> and think about newer versions of Linux headers which GRUB is
> not aware of.
>
>> As I understand it, the read is not harmful (as it just gets
>> overwritten by the bootloader later) but also not needed. Is this your
>> understanding?
>
> This read is needed, however, math has to be fixed. Additionally,
> I think that you can take a look at SYSLINUX and do something
> similar in GRUB.
>
>> I'll definitely submit a patch upstream once I figure out what should
>> be going on.
>
> Great! Thanks!
>
> Daniel

Re: What is this grub_disk_read doing in the i386 linux loader?

[Michael Chang]

On Wed, Apr 25, 2018 at 12:45:15PM -0700, Andrew Jeddeloh wrote:
> > As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.
> 
> I think Michael's message got lost; I think he replied to just you? I
> didn't receive it and I don't see it in the archives.

Yes, it got lost. I don't know why, but it did have the mailing list address in
the "To" recipient. It was done by simply making a "Group Reply" to your
previous mail from mutt, works for me every time, and also no obvious
errors/warning observed during sending the email via MTA. 

Anyway here is the body of lost message.

"
Hi,

The boot spec has proposed the way to do it.

  The end of setup header can be calculated as follow:
  0x0202 + byte value at offset 0x0201"

IMHO the linux_params struct is a superset of setup header that you could
inadvertently overwrite unwanted fields like the e820_map you may already know.
"

Thanks,
Michael

> 
> > AIUI it is. Please look above.
> 
> I don't know what you're refering to here by "above".
> 
> > This is wrong due to grub_e820_mmap.
> 
> My understanding is wrong or grub's behavior is wrong? Can you elaborate please?
> 
> > This read is needed
> 
> I don't understand why. Again, looking at the zero page documentation,
> there's nothing after the 16 bit boot params that the linux image
> could know ahead of time. Is your concern that future fields could be
> added that do need to be read?
> 
> Regardless, we can't calculate then length then blindly use it reading
> into a struct. If the length grows larger than the size of the struct
> we'll start corrupting memory when doing the read. Assuming the read
> is needed, we'll need to malloc the amount that still needs to be read
> then probably copy that into a struct to modify (and ignore the bits
> we don't understand in the case of a new Linux release adding fields
> grub doesn't have yet).
> 
> Sorry for all the questions, I want to ensure that I understand what's
> supposed to be happening first.
> 
> Thanks,
>  - Andrew
> 
> On Wed, Apr 25, 2018 at 1:59 AM, Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Tue, Apr 24, 2018 at 04:08:57PM -0700, Andrew Jeddeloh wrote:
> >> Thanks for the reply.
> >>
> >> I'm not sure I follow. Looking over the 32 bit boot spec, it looks
> >> like the process is:
> >>
> >> 1) zero out linux_params
> >>  - grub does this
> >> 2) copy the linux boot params (from 0x1f1) into linux params
> >>  - grub does this by reading from 0x0 until the end of lh, then
> >> copying lh+0x1f1 til the end of lh into linux_headers [4][5]
> >> 3) Do the read/write/modify operations that would happen for a 16 bit boot
> >> 4) calculate the end of the the setup header
> >>  - grub does not do this and I think assumes its just the end of the
> >> linux_params struct
> >
> > As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.
> >
> >> 5) fill out the rest of the setup header things from the zero page doc [6]
> >>  - AFAICT this isn't meant to be read from the kernel image, but
> >> instead filled out by bootloader or left as zero
> >
> > AIUI it is. Please look above.
> >
> >>    - It looks like the current code is trying to do this but is just
> >> assuming the end is at the end of linux_params.
> >
> > This is wrong due to grub_e820_mmap.
> >
> >>  - Looking over the items in the zero page doc, there doesn't seem to
> >> be anything that the kernel could supply a useful "default" value.
> >
> > GRUB have to load the rest of Linux header into linux_params.
> > Current math is wrong and have to be fixed. Please look above
> > and think about newer versions of Linux headers which GRUB is
> > not aware of.
> >
> >> As I understand it, the read is not harmful (as it just gets
> >> overwritten by the bootloader later) but also not needed. Is this your
> >> understanding?
> >
> > This read is needed, however, math has to be fixed. Additionally,
> > I think that you can take a look at SYSLINUX and do something
> > similar in GRUB.
> >
> >> I'll definitely submit a patch upstream once I figure out what should
> >> be going on.
> >
> > Great! Thanks!
> >
> > Daniel

Re: What is this grub_disk_read doing in the i386 linux loader?

[Daniel Kiper]

On Wed, Apr 25, 2018 at 12:45:15PM -0700, Andrew Jeddeloh wrote:
> > As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.
>
> I think Michael's message got lost; I think he replied to just you? I
> didn't receive it and I don't see it in the archives.
>
> > AIUI it is. Please look above.
>
> I don't know what you're refering to here by "above".

I am referring to the math above and in general to 32-bit BOOT PROTOCOL
paragraph from https://www.kernel.org/doc/Documentation/x86/boot.txt

> > This is wrong due to grub_e820_mmap.
>
> My understanding is wrong or grub's behavior is wrong? Can you elaborate please?

GRUB behavior is wrong due to grub_e820_mmap.

> > This read is needed
>
> I don't understand why. Again, looking at the zero page documentation,
> there's nothing after the 16 bit boot params that the linux image

I am not sure what do you mean by "16 bit boot params". Anyway, again 32-bit
BOOT PROTOCOL paragraph is pretty clear: ... Then the setup header from
offset 0x01f1 of kernel image on should be loaded into struct boot_params
and examined. The end of setup header can be calculated as follow:
0x0202 + byte value at offset 0x0201 ...

> could know ahead of time. Is your concern that future fields could be
> added that do need to be read?

Exactly.

> Regardless, we can't calculate then length then blindly use it reading
> into a struct. If the length grows larger than the size of the struct
> we'll start corrupting memory when doing the read. Assuming the read

Yep, the end of linux_params.pad2 is the limit. We should not go
over it. So, just simply check that we are not going to read more
than that. Otherwise fail and print an error message.

> is needed, we'll need to malloc the amount that still needs to be read
> then probably copy that into a struct to modify (and ignore the bits
> we don't understand in the case of a new Linux release adding fields
> grub doesn't have yet).
>
> Sorry for all the questions, I want to ensure that I understand what's

No problem.

> supposed to be happening first.

That is good way to go.

Daniel