Re: [coreboot] Dell Dimension 8300 reboots when grub2 cbfs module is loaded

[Andrei Borzenkov]

04.11.2015 00:41, Nico Huber пишет:
> Hi Andrei,
>
> your patch looks good generally, but the check is off by one. It's

Yes, but it should not matter in real life. Last 4 bytes are pointer, so 
header cannot start after 0xffffffdc anyway. I'll change it.

> obvious, we want to have sane checking there. Reading from a random
> address can cause trouble and 0xffffffff is not the only offending
> address.
>
> On x86, the cbfs is mapped right below the 4GiB line. Current machines
> don't have more than 16MiB space for cbfs, FWIW. So maybe it's best to
> check if the ptr points somewhere between 0xff000000 and (0x100000000 -
> sizeof(*head)).

That's far exceeds my intention and knowledge, sorry.

> Nico
>
> On 01.11.2015 15:53, Andrei Borzenkov wrote:
> > I was debugging problem reported by user on Dell Dimension 8300 - it
> > rebooted when doing "ls -l". It turned out, the problem was triggered by
> > loading cbfs which probed for header. System has 2GB memory, and attempt
> > to read from address 0xffffffff caused instant reboot. 0xffffffff was
> > returned by read from non-existing address 0xfffffffc.
> >
> > The proof of concept patch below avoids it, but I wonder what the proper
> > fix is.
> >
> > diff --git a/grub-core/fs/cbfs.c b/grub-core/fs/cbfs.c
> > index a34eb88..a5a2fde 100644
> > --- a/grub-core/fs/cbfs.c
> > +++ b/grub-core/fs/cbfs.c
> > @@ -344,8 +344,9 @@ init_cbfsdisk (void)
> >
> >     ptr = *(grub_uint32_t *) 0xfffffffc;
> >     head = (struct cbfs_header *) (grub_addr_t) ptr;
> > +  grub_dprintf ("cbfs", "head=%p\n", head);
> >
> > -  if (!validate_head (head))
> > +  if (0xffffffff - ptr < sizeof (*head) || !validate_head (head))
> >       return;
> >
> >     cbfsdisk_size = ALIGN_UP (grub_be_to_cpu32 (head->romsize),