[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

segfault in grub_device_iterate on 64bit platforms

From: Dan Merillat
Subject: segfault in grub_device_iterate on 64bit platforms
Date: Tue, 19 Jan 2010 20:23:44 -0500
User-agent: Mozilla-Thunderbird (X11/20090707)

Yo Grub2, I'm really happy for you and I'mma let you finish but...
allocating sizeof(p->next) instead of sizeof(*p) is the worst idiom of
all time.  OF ALL TIME.

</Kayne West>

Diff against debian's 1.98-experimental-20100111, to which I submit the
following patches:

diff --git a/kern/device.c b/kern/device.c
index d4de496..3727ddc 100644
--- a/kern/device.c
+++ b/kern/device.c
@@ -139,7 +139,7 @@ grub_device_iterate (int (*hook) (const char *name))
       if (! partition_name)
        return 1;

-      p = grub_malloc (sizeof (p->next));
+      p = grub_malloc (sizeof (*p));
       if (!p)
          grub_free (partition_name);

Seriously though, where did someone see sizeof(p->next) and think it was
a valid idiom?  It allocates a pointer, not a structure.  I've never
seen it used before, and a quick rgrep shows it to be an anomaly in the
grub source as well.  It happens to work on 32bit processors for tiny
structures because the default alignment for malloc is 8 bytes - and
struct part_ent is two pointers.  You end up overflowing into the
padding and not trashing anything.

On 64bit, the structure is 16 bytes long, and trashes whatever the next
allocation is, which leads to nasty problems and random crashes later.

Electric Fence.  Learn it, love it, live it.

And here's a freebie to avoid a null pointer deref that -lefence pointed

diff --git a/util/grub-probe.c b/util/grub-probe.c
index 4ba8a98..acb0887 100644
--- a/util/grub-probe.c
+++ b/util/grub-probe.c
@@ -94,6 +94,8 @@ probe_partmap (grub_disk_t disk)
 static int
 probe_raid_level (grub_disk_t disk)
+  if (!disk)
+         return -1;
   if (disk->dev->id != GRUB_DISK_DEVICE_RAID_ID)
     return -1;

Someone may want to look into why probe_raid_level can get passed an
invalid disk pointer, but the combo of these two patches makes it work
properly.  I don't know if it's possible to have a disk pointer without
a dev pointer, but that's a question for people with some deeper knowledge.

This may fix some of the other bug reports - I see stacktraces crossing
grub_device_iterate which tells me that this is a potential corruption
in all of them.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]