[PATCH] segfault in grub-probe when spare or faulty disks are found

[PATCH] segfault in grub-probe when spare or faulty disks are found

[Martin Schitter]

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

the actual raid implementation doesn't handle spare or faulty drives in 
a linux software raid array in a sane way. grub-probe will reproducible 
crash in such a setup.

please include the attached trivial patch to fix this problem.

for more information see:

https://savannah.gnu.org/bugs/?31119
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=593652


[-- Attachment #2: grub2-raid-better.diff --]
[-- Type: text/x-patch, Size: 1094 bytes --]

=== modified file 'grub-core/disk/raid.c'
--- grub-core/disk/raid.c	2010-09-13 21:59:22 +0000
+++ grub-core/disk/raid.c	2010-09-25 22:20:29 +0000
@@ -501,7 +501,8 @@
           grub_dprintf ("raid", "array->nr_devs > array->total_devs (%d)?!?",
 			array->total_devs);
 
-        if (array->device[new_array->index] != NULL)
+        if ((new_array->index < GRUB_RAID_MAX_DEVICES) &&
+	    (array->device[new_array->index] != NULL))
           /* We found multiple devices with the same number. Again,
              this shouldn't happen.  */
           grub_dprintf ("raid", "Found two disks with the number %d?!?",
@@ -609,9 +610,14 @@
     }
 
   /* Add the device to the array. */
-  array->device[new_array->index] = disk;
-  array->start_sector[new_array->index] = start_sector;
-  array->nr_devs++;
+
+  /* ignore spare/faulty disks and more then GRUB_RAID_MAX_DEVICES */
+  if (new_array->index < GRUB_RAID_MAX_DEVICES)
+    {
+      array->device[new_array->index] = disk;
+      array->start_sector[new_array->index] = start_sector;
+      array->nr_devs++;
+    }
 
   return 0;
 }

Re: [PATCH] segfault in grub-probe when spare or faulty disks are found

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 09/27/2010 10:32 PM, Martin Schitter wrote:
> the actual raid implementation doesn't handle spare or faulty drives
> in a linux software raid array in a sane way. grub-probe will
> reproducible crash in such a setup.
>
> please include the attached trivial patch to fix this problem.
>
I didn't respond to the patch because it's not correct: it only
propagates the restriction instead of removing it. Thanks for reporting
the issue. I'll fix it tomorrow (hopefully)
> for more information see:
>
> https://savannah.gnu.org/bugs/?31119
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=593652
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] segfault in grub-probe when spare or faulty disks are found

[Martin Schitter]

Am 2010-09-28 22:24, schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
> I didn't respond to the patch because it's not correct: it only
> propagates the restriction instead of removing it. Thanks for reporting
> the issue. I'll fix it tomorrow (hopefully)

yes, there are more consequent ways to solve the problem.
my simple patch doesn't distinguish between all the special cases:

  index == 0xffff: spare disk,
  index == 0xfffe: faulty disk
  index out of range (>= GRUB_RAID_MAX_DEVICES).

it makes a very simple test and ignores all affected devices in any 
case. isn't that enough to prevent the segmentation fault?

it would be nice, if you could find a better workaround.

Re: [PATCH] segfault in grub-probe when spare or faulty disks are found

[Martin Schitter]

Am 2010-09-28 22:24, schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
> I didn't respond to the patch because it's not correct: it only
> propagates the restriction instead of removing it. Thanks for reporting
> the issue. I'll fix it tomorrow (hopefully)

the problem with version 1.x raid spare disks still exists (see also: 
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=593652).
could you please come up with a better solution for this problem or 
correct my patch.

thanks
martin

Re: [PATCH] segfault in grub-probe when spare or faulty disks are found

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 10/25/2010 01:07 PM, Martin Schitter wrote:
> Am 2010-09-28 22:24, schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
>> I didn't respond to the patch because it's not correct: it only
>> propagates the restriction instead of removing it. Thanks for reporting
>> the issue. I'll fix it tomorrow (hopefully)
>
> the problem with version 1.x raid spare disks still exists (see also:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=593652).
> could you please come up with a better solution for this problem or
> correct my patch.
>
Sorry for the delay, I had very busy period. I'll do this this evening
> thanks
> martin
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] segfault in grub-probe when spare or faulty disks are found

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 682 bytes --]

On 09/27/2010 10:32 PM, Martin Schitter wrote:
> the actual raid implementation doesn't handle spare or faulty drives
> in a linux software raid array in a sane way. grub-probe will
> reproducible crash in such a setup.
>
> please include the attached trivial patch to fix this problem.
>
Please try the attached patch
> for more information see:
>
> https://savannah.gnu.org/bugs/?31119
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=593652
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: raid.diff --]
[-- Type: text/x-diff; name="raid.diff", Size: 11187 bytes --]

=== modified file 'grub-core/disk/mdraid1x_linux.c'
--- grub-core/disk/mdraid1x_linux.c	2010-09-20 18:09:31 +0000
+++ grub-core/disk/mdraid1x_linux.c	2010-10-26 08:54:53 +0000
@@ -188,12 +188,14 @@
 	array->total_devs = grub_le_to_cpu32 (real_sb->raid_disks);
 	array->disk_size = grub_le_to_cpu64 (real_sb->size);
 	array->chunk_size = grub_le_to_cpu32 (real_sb->chunksize);
-	if (grub_le_to_cpu32 (real_sb->dev_number) <
+
+	if (grub_le_to_cpu32 (real_sb->dev_number) >=
 	    grub_le_to_cpu32 (real_sb->max_dev))
-	  array->index = grub_le_to_cpu16
-	    (real_sb->dev_roles[grub_le_to_cpu32 (real_sb->dev_number)]);
-	else
-	  array->index = 0xffff;  /* disk will be later not used! */
+	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+			     "spares aren't implemented");
+
+	array->index = grub_le_to_cpu16
+	  (real_sb->dev_roles[grub_le_to_cpu32 (real_sb->dev_number)]);
 	array->uuid_len = 16;
 	array->uuid = grub_malloc (16);
 	if (!array->uuid)

=== modified file 'grub-core/disk/mdraid_linux.c'
--- grub-core/disk/mdraid_linux.c	2010-09-20 18:09:31 +0000
+++ grub-core/disk/mdraid_linux.c	2010-10-26 08:53:39 +0000
@@ -194,6 +194,9 @@
       sb.level != 5 && sb.level != 6 && sb.level != 10)
     return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		       "unsupported RAID level: %d", sb.level);
+  if (sb.this_disk.number == 0xffff || sb.this_disk.number == 0xfffe)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+		       "spares aren't implemented");
 
   array->name = NULL;
   array->number = sb.md_minor;

=== modified file 'grub-core/disk/raid.c'
--- grub-core/disk/raid.c	2010-10-08 21:27:27 +0000
+++ grub-core/disk/raid.c	2010-10-26 08:52:04 +0000
@@ -97,10 +97,10 @@
   unsigned int i;
 
   for (i = 0; i < array->total_devs; i++)
-    if (array->device[i])
+    if (array->members[i].device)
       {
         tmp = grub_malloc (sizeof (*tmp));
-        tmp->disk = array->device[i];
+        tmp->disk = array->members[i].device;
         tmp->next = list;
         list = tmp;
       }
@@ -255,13 +255,13 @@
                 k = disknr;
                 for (j = 0; j < far; j++)
                   {
-                    if (array->device[k])
+                    if (array->members[k].device)
                       {
                         if (grub_errno == GRUB_ERR_READ_ERROR)
                           grub_errno = GRUB_ERR_NONE;
 
-                        err = grub_disk_read (array->device[k],
-                                              array->start_sector[k] +
+                        err = grub_disk_read (array->members[k].device,
+                                              array->members[k].start_sector +
                                                 read_sector + j * far_ofs + b,
                                               0,
                                               read_size << GRUB_DISK_SECTOR_BITS,
@@ -367,14 +367,14 @@
               read_size = size;
 
             e = 0;
-            if (array->device[disknr])
+            if (array->members[disknr].device)
               {
                 /* Reset read error.  */
                 if (grub_errno == GRUB_ERR_READ_ERROR)
                   grub_errno = GRUB_ERR_NONE;
 
-                err = grub_disk_read (array->device[disknr],
-                                      array->start_sector[disknr] +
+                err = grub_disk_read (array->members[disknr].device,
+                                      array->members[disknr].start_sector +
                                         read_sector + b, 0,
                                       read_size << GRUB_DISK_SECTOR_BITS,
                                       buf);
@@ -500,6 +500,21 @@
 
         /* Do some checks before adding the device to the array.  */
 
+	if (new_array->index >= array->allocated_devs)
+	  {
+	    void *tmp;
+	    unsigned int newnum = 2 * (new_array->index + 1);
+	    tmp = grub_realloc (array->members, newnum
+				* sizeof (array->members[0]));
+	    if (!tmp)
+	      return grub_errno;
+	    array->members = tmp;
+	    grub_memset (array->members + array->allocated_devs,
+			 0, (newnum - array->allocated_devs)
+			 * sizeof (array->members[0]));
+	    array->allocated_devs = newnum;
+	  }
+
         /* FIXME: Check whether the update time of the superblocks are
            the same.  */
 
@@ -510,7 +525,7 @@
           grub_dprintf ("raid", "array->nr_devs > array->total_devs (%d)?!?",
 			array->total_devs);
 
-        if (array->device[new_array->index] != NULL)
+        if (array->members[new_array->index].device != NULL)
           /* We found multiple devices with the same number. Again,
              this shouldn't happen.  */
           grub_dprintf ("raid", "Found two disks with the number %d?!?",
@@ -536,8 +551,18 @@
 #ifdef GRUB_UTIL
       array->driver = raid;
 #endif
-      grub_memset (&array->device, 0, sizeof (array->device));
-      grub_memset (&array->start_sector, 0, sizeof (array->start_sector));
+      array->allocated_devs = 32;
+      if (new_array->index >= array->allocated_devs)
+	array->allocated_devs = 2 * (new_array->index + 1);
+
+      array->members = grub_zalloc (array->allocated_devs
+				    * sizeof (array->members[0]));
+
+      if (!array->members)
+        {
+          grub_free (new_array->uuid);
+          return grub_errno;
+        }
 
       if (! array->name)
 	{
@@ -582,6 +607,7 @@
 	  array->name = grub_xasprintf ("md%d", array->number);
 	  if (! array->name)
 	    {
+	      grub_free (array->members);
 	      grub_free (array->uuid);
 	      grub_free (array);
 
@@ -597,6 +623,7 @@
 
 	  if (! new_name)
 	    {
+	      grub_free (array->members);
 	      grub_free (array->uuid);
 	      grub_free (array);
 
@@ -621,8 +648,8 @@
     }
 
   /* Add the device to the array. */
-  array->device[new_array->index] = disk;
-  array->start_sector[new_array->index] = start_sector;
+  array->members[new_array->index].device = disk;
+  array->members[new_array->index].start_sector = start_sector;
   array->nr_devs++;
 
   return 0;
@@ -639,14 +666,15 @@
   while (array)
     {
       struct grub_raid_array *p;
-      int i;
+      unsigned int i;
 
       p = array;
       array = array->next;
 
-      for (i = 0; i < GRUB_RAID_MAX_DEVICES; i++)
-        if (p->device[i])
-          grub_disk_close (p->device[i]);
+      for (i = 0; i < p->allocated_devs; i++)
+        if (p->members[i].device)
+          grub_disk_close (p->members[i].device);
+      grub_free (p->members);
 
       grub_free (p->uuid);
       grub_free (p->name);

=== modified file 'grub-core/disk/raid5_recover.c'
--- grub-core/disk/raid5_recover.c	2009-07-31 04:38:20 +0000
+++ grub-core/disk/raid5_recover.c	2010-10-26 08:34:44 +0000
@@ -45,7 +45,7 @@
       if (i == disknr)
         continue;
 
-      err = grub_disk_read (array->device[i], sector, 0, size, buf2);
+      err = grub_disk_read (array->members[i].device, sector, 0, size, buf2);
 
       if (err)
         {

=== modified file 'grub-core/disk/raid6_recover.c'
--- grub-core/disk/raid6_recover.c	2010-01-03 22:05:07 +0000
+++ grub-core/disk/raid6_recover.c	2010-10-26 08:38:03 +0000
@@ -118,8 +118,9 @@
         bad1 = i;
       else
         {
-          if ((array->device[pos]) &&
-              (! grub_disk_read (array->device[pos], sector, 0, size, buf)))
+          if ((array->members[pos].device) &&
+              (! grub_disk_read (array->members[pos].device, sector,
+				 0, size, buf)))
             {
               grub_raid_block_xor (pbuf, buf, size);
               grub_raid_block_mul (raid6_table2[i][i], buf, size);
@@ -148,21 +149,21 @@
   if (bad2 < 0)
     {
       /* One bad device */
-      if ((array->device[p]) &&
-          (! grub_disk_read (array->device[p], sector, 0, size, buf)))
+      if ((array->members[p].device) &&
+          (! grub_disk_read (array->members[p].device, sector, 0, size, buf)))
         {
           grub_raid_block_xor (buf, pbuf, size);
           goto quit;
         }
 
-      if (! array->device[q])
+      if (! array->members[q].device)
         {
           grub_error (GRUB_ERR_READ_ERROR, "not enough disk to restore");
           goto quit;
         }
 
       grub_errno = GRUB_ERR_NONE;
-      if (grub_disk_read (array->device[q], sector, 0, size, buf))
+      if (grub_disk_read (array->members[q].device, sector, 0, size, buf))
         goto quit;
 
       grub_raid_block_xor (buf, qbuf, size);
@@ -174,18 +175,18 @@
       /* Two bad devices */
       grub_uint8_t c;
 
-      if ((! array->device[p]) || (! array->device[q]))
+      if ((! array->members[p].device) || (! array->members[q].device))
         {
           grub_error (GRUB_ERR_READ_ERROR, "not enough disk to restore");
           goto quit;
         }
 
-      if (grub_disk_read (array->device[p], sector, 0, size, buf))
+      if (grub_disk_read (array->members[p].device, sector, 0, size, buf))
         goto quit;
 
       grub_raid_block_xor (pbuf, buf, size);
 
-      if (grub_disk_read (array->device[q], sector, 0, size, buf))
+      if (grub_disk_read (array->members[q].device, sector, 0, size, buf))
         goto quit;
 
       grub_raid_block_xor (qbuf, buf, size);

=== modified file 'include/grub/raid.h'
--- include/grub/raid.h	2010-10-08 21:27:27 +0000
+++ include/grub/raid.h	2010-10-26 08:55:53 +0000
@@ -22,8 +22,6 @@
 
 #include <grub/types.h>
 
-#define GRUB_RAID_MAX_DEVICES	32
-
 #define GRUB_RAID_LAYOUT_LEFT_ASYMMETRIC	0
 #define GRUB_RAID_LAYOUT_RIGHT_ASYMMETRIC	1
 #define GRUB_RAID_LAYOUT_LEFT_SYMMETRIC		2
@@ -32,6 +30,13 @@
 #define GRUB_RAID_LAYOUT_RIGHT_MASK		1
 #define GRUB_RAID_LAYOUT_SYMMETRIC_MASK		2
 
+struct grub_raid_member
+{
+  grub_disk_t device;  /* Array of total_devs devices. */
+  grub_disk_addr_t start_sector;
+  /* Start of each device, in 512 byte sectors. */
+};
+
 struct grub_raid_array
 {
   int number;              /* The device number, taken from md_minor so we
@@ -43,16 +48,15 @@
   grub_size_t chunk_size;  /* The size of a chunk, in 512 byte sectors. */
   grub_uint64_t disk_size; /* Size of an individual disk, in 512 byte
 			      sectors. */
-  int index;               /* Index of current device.  */
+  unsigned int index;               /* Index of current device.  */
   int uuid_len;            /* The length of uuid.  */
   char *uuid;              /* The UUID of the device. */
 
   /* The following field is setup by the caller.  */
   char *name;              /* That will be "md<number>". */
   unsigned int nr_devs;    /* The number of devices we've found so far. */
-  grub_disk_t device[GRUB_RAID_MAX_DEVICES];  /* Array of total_devs devices. */
-  grub_disk_addr_t start_sector[GRUB_RAID_MAX_DEVICES];
-			   /* Start of each device, in 512 byte sectors. */
+  unsigned int allocated_devs;
+  struct grub_raid_member *members;
   struct grub_raid_array *next;
 
 #ifdef GRUB_UTIL


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] segfault in grub-probe when spare or faulty disks are found

[Martin Schitter]

Am 2010-10-26 10:58, schrieb Vladimir 'φ-coder/phcoder' Serbinenko:
> Please try the attached patch

yes, this patch seems to fix the problem!

thanks!

martin