[PATCH] Fix potential truncation of mdraid device list

[PATCH] Fix potential truncation of mdraid device list

[Kees Cook]

The consumer of grub_util_raid_getmembers() expects a NULL terminated list
of device names. It has no idea about how many devices are registered in
the array; it only cares about active devices. As a result, there cannot
be gaps in the list, otherwise the first listed inactive device will cause
all remaining devices to effectively vanish. This is especially
troublesome if a root filesystem were on an array with the first device
being a hot spare: the array would appear to have no disks and the root
filesystem would become invisible to grub.

Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed" disks")
Fixes: https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1912043
Fixes: https://savannah.gnu.org/bugs/index.php?59887
Signed-off-by: Kees Cook <kees@ubuntu.com>

Index: grub2-2.04/grub-core/osdep/linux/getroot.c
===================================================================
--- grub2-2.04.orig/grub-core/osdep/linux/getroot.c
+++ grub2-2.04/grub-core/osdep/linux/getroot.c
@@ -170,21 +170,21 @@ grub_util_raid_getmembers (const char *n
 
   devicelist = xcalloc (info.nr_disks + 1, sizeof (char *));
 
-  for (i = 0, j = 0; j < info.nr_disks; i++)
+  for (i = 0, j = 0; i < info.nr_disks; i++)
     {
       disk.number = i;
       ret = ioctl (fd, GET_DISK_INFO, &disk);
       if (ret != 0)
 	grub_util_error (_("ioctl GET_DISK_INFO error: %s"), strerror (errno));
-      
+
       if (disk.state & (1 << MD_DISK_REMOVED))
 	continue;
 
-      if (disk.state & (1 << MD_DISK_ACTIVE))
-	devicelist[j] = grub_find_device (NULL,
-					  makedev (disk.major, disk.minor));
-      else
-	devicelist[j] = NULL;
+      if (!(disk.state & (1 << MD_DISK_ACTIVE)))
+	continue;
+
+      devicelist[j] = grub_find_device (NULL,
+					makedev (disk.major, disk.minor));
       j++;
     }
 

-- 
Kees Cook                                            @outflux.net

Re: [PATCH] Fix potential truncation of mdraid device list

[Petr Vorel]

Hi,

> The consumer of grub_util_raid_getmembers() expects a NULL terminated list
> of device names. It has no idea about how many devices are registered in
> the array; it only cares about active devices. As a result, there cannot
> be gaps in the list, otherwise the first listed inactive device will cause
> all remaining devices to effectively vanish. This is especially
> troublesome if a root filesystem were on an array with the first device
> being a hot spare: the array would appear to have no disks and the root
> filesystem would become invisible to grub.

> Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed" disks")
> Fixes: https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1912043
> Fixes: https://savannah.gnu.org/bugs/index.php?59887
> Signed-off-by: Kees Cook <kees@ubuntu.com>

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> Index: grub2-2.04/grub-core/osdep/linux/getroot.c
> ===================================================================
> --- grub2-2.04.orig/grub-core/osdep/linux/getroot.c
> +++ grub2-2.04/grub-core/osdep/linux/getroot.c
> @@ -170,21 +170,21 @@ grub_util_raid_getmembers (const char *n

>    devicelist = xcalloc (info.nr_disks + 1, sizeof (char *));

> -  for (i = 0, j = 0; j < info.nr_disks; i++)
> +  for (i = 0, j = 0; i < info.nr_disks; i++)
>      {
>        disk.number = i;
>        ret = ioctl (fd, GET_DISK_INFO, &disk);
>        if (ret != 0)
>  	grub_util_error (_("ioctl GET_DISK_INFO error: %s"), strerror (errno));
> -      
> +
>        if (disk.state & (1 << MD_DISK_REMOVED))
>  	continue;

> -      if (disk.state & (1 << MD_DISK_ACTIVE))
> -	devicelist[j] = grub_find_device (NULL,
> -					  makedev (disk.major, disk.minor));
> -      else
> -	devicelist[j] = NULL;
> +      if (!(disk.state & (1 << MD_DISK_ACTIVE)))
> +	continue;
> +
> +      devicelist[j] = grub_find_device (NULL,
> +					makedev (disk.major, disk.minor));
>        j++;
>      }