Dell Dimension 8300 reboots when grub2 cbfs module is loaded

Dell Dimension 8300 reboots when grub2 cbfs module is loaded

[Andrei Borzenkov]

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

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

[Martin Roth]

Thanks Andrei,

Vladimir, what do you think?

Martin

On Sun, Nov 1, 2015 at 7:53 AM, Andrei Borzenkov <arvidjaar@gmail.com> 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),
>
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot

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

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

The code itself looks good but I'd like more details. Reading 0xffffffff
shouldn't cause reboot. Why does it?
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
>

[-- Attachment #2: Type: text/html, Size: 1824 bytes --]

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

[Andrei Borzenkov]

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
>

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

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

Which platform is it? i386-pc, i386-efi or x86_64-efi? The behavior is
actually will defined, just different between cpu modes
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
>

[-- Attachment #2: Type: text/html, Size: 3886 bytes --]

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

[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

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

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

[Andrei Borzenkov]

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

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

[Aaron Durbin]

On Tue, Nov 3, 2015 at 10:28 AM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> The code itself looks good but I'd like more details. Reading 0xffffffff
> shouldn't cause reboot. Why does it?

It's probably implementation defined reading a multi-byte object from
4GiB-1. Does it wrap? Blow up the machine? Machine check? Transaction
never gets terminated?

>
> 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
>
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot

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

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

Le 3 nov. 2015 6:46 PM, "Aaron Durbin" <adurbin@google.com> a écrit :
>
> On Tue, Nov 3, 2015 at 10:28 AM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > The code itself looks good but I'd like more details. Reading 0xffffffff
> > shouldn't cause reboot. Why does it?
>
> It's probably implementation defined reading a multi-byte object from
> 4GiB-1. Does it wrap? Blow up the machine? Machine check? Transaction
> never gets terminated?
>
It would be interesting to find out. Since it's P4, it may be related to
PAE but paging is disabled when GRUB is active However it shouldn't hold
this patch. Andrei: go ahead, just please add reference to machine and cpu
in the comment and the fact that we have little idea what's going on.
> >
> > 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
> >
> >
> > --
> > coreboot mailing list: coreboot@coreboot.org
> > http://www.coreboot.org/mailman/listinfo/coreboot

[-- Attachment #2: Type: text/html, Size: 3239 bytes --]

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

[Nico Huber]

Hi Andrei,

your patch looks good generally, but the check is off by one. It's
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)).

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

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