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 <address@hidden> 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