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