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),
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
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
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),
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),
-
Andrei Borzenkov -
Martin Roth -
Nico Huber -
Vladimir 'phcoder' Serbinenko