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

[Andrei Borzenkov <arvidjaar@gmail.com>]

03.11.2015 20:14, Andrei Borzenkov пишет:
> 03.11.2015 20:10, Vladimir 'phcoder' Serbinenko пишет:
>> Which platform is it? i386-pc, i386-efi or x86_64-efi? The behavior is
>> actually will defined, just different between cpu modes
>
> i386-pc
>

BTW here are specs from Dell site

Intel® Pentium® 4 microprocessor (2.4, 2.6, 2.8, 3.0,
3.2, and 3.4 for 800 FSB, and 2.4, 2.66, 2.8, and 3.06
for 533 FSB)

4GB max memory

Intel 875P chipset

>> Le 3 nov. 2015 6:08 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> a
>> écrit :
>>
>>> 03.11.2015 19:28, Vladimir 'phcoder' Serbinenko пишет:
>>>
>>>> The code itself looks good but I'd like more details. Reading
>>>> 0xffffffff
>>>> shouldn't cause reboot. Why does it?
>>>>
>>>
>>> That I do not know nor do I have access to system in question myself. I
>>> sent user patch that modified validate_header to do each comparison as
>>> individual statement and did line by line debug print (fortunately it
>>> was
>>> possible to connect serial port and capture output) and the last line
>>> printed was immediately before the very first
>>>
>>> head->magic == grub_cpu_to_be32_compile_time (CBFS_HEADER_MAGIC
>>>
>>> I suppose reading *one* byte from 0xffffffff should not cause issues but
>>> here we are reading 4 bytes which are beyond 0xffffffff. Who knows what
>>> memory controller in this system does in this case.
>>>
>>> Le 1 nov. 2015 3:53 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> a
>>> écrit :
>>>>
>>>> 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),
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Grub-devel mailing list
>>>>> Grub-devel@gnu.org
>>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>