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

[Andrei Borzenkov <arvidjaar@gmail.com>]

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),
>>
>>