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