* [PATCH] Fix md RAID enumeration
@ 2021-01-16 17:27 Kees Cook
2021-01-17 21:38 ` [PATCH] osdep/linux: Fix md array device enumeration Kees Cook
0 siblings, 1 reply; 2+ messages in thread
From: Kees Cook @ 2021-01-16 17:27 UTC (permalink / raw
To: grub-devel
In addition to the prior patch to avoid leaving holes in the returned
devicelist, the enumeration logic also needed fixing. GET_ARRAY_INFO's
info.nr_disks does not map to GET_DISK_INFO's disk.number, which is an
internal kernel index. If an array has had drives added, removed, etc,
there may be gaps in GET_DISK_INFO's results. Instead, do what mdadm does
and keep scanning until we find enough valid (major/minor != 0) disks.
Fixes: 2b00217369ac ("... Added support for RAID and LVM")
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>
(This is intended to be applied on top of the earlier patch sent in
https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00027.html)
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
@@ -135,10 +135,18 @@ struct mountinfo_entry
char fstype[ESCAPED_PATH_MAX + 1], device[ESCAPED_PATH_MAX + 1];
};
+/* GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
+ disk.number, which is an internal kernel index. Instead, do what mdadm
+ does and keep scanning until we find enough valid disks. The limit is
+ copied from there, which notes that it is sufficiently high given
+ that the on-disk metadata for v1.x can only support 1920. */
+#define MD_MAX_DISKS 4096
+
static char **
grub_util_raid_getmembers (const char *name, int bootable)
{
int fd, ret, i, j;
+ int remaining;
char **devicelist;
mdu_version_t version;
mdu_array_info_t info;
@@ -170,12 +178,17 @@ grub_util_raid_getmembers (const char *n
devicelist = xcalloc (info.nr_disks + 1, sizeof (char *));
- for (i = 0, j = 0; i < info.nr_disks; i++)
+ remaining = info.nr_disks;
+ for (i = 0, j = 0; i < MD_MAX_DISKS && remaining > 0; 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));
+ /* Skip empty disk slot. */
+ if (disk.major == 0 && disk.minor == 0)
+ continue;
+ remaining--;
if (disk.state & (1 << MD_DISK_REMOVED))
continue;
--
Kees Cook @outflux.net
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH] osdep/linux: Fix md array device enumeration
2021-01-16 17:27 [PATCH] Fix md RAID enumeration Kees Cook
@ 2021-01-17 21:38 ` Kees Cook
0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2021-01-17 21:38 UTC (permalink / raw
To: grub-devel
GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
disk.number, which is an internal kernel index. If an array has had drives
added, removed, etc, there may be gaps in GET_DISK_INFO's results. But
since the consumer of devicelist cannot tolerate gaps (it expects to walk
a NULL-terminated list of device name strings), the devicelist index (j)
must be tracked separately from the disk.number index (i). But grub
wants to only examine active (i.e. present and non-failed) disks, so how
many disks are left to be queried must be also separately tracked
(remaining).
Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed" disks")
Fixes: 2b00217369ac ("... Added support for RAID and LVM")
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>
---
Actually, here's a better patch which combines the two I sent...
---
grub-core/osdep/linux/getroot.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/grub-core/osdep/linux/getroot.c b/grub-core/osdep/linux/getroot.c
index 001b818fe581..d7b62e702e1f 100644
--- a/grub-core/osdep/linux/getroot.c
+++ b/grub-core/osdep/linux/getroot.c
@@ -135,10 +135,18 @@ struct mountinfo_entry
char fstype[ESCAPED_PATH_MAX + 1], device[ESCAPED_PATH_MAX + 1];
};
+/* GET_DISK_INFO nr_disks (total count) does not map to disk.number,
+ which is an internal kernel index. Instead, do what mdadm does
+ and keep scanning until we find enough valid disks. The limit is
+ copied from there, which notes that it is sufficiently high given
+ that the on-disk metadata for v1.x can only support 1920. */
+#define MD_MAX_DISKS 4096
+
static char **
grub_util_raid_getmembers (const char *name, int bootable)
{
int fd, ret, i, j;
+ int remaining;
char **devicelist;
mdu_version_t version;
mdu_array_info_t info;
@@ -170,22 +178,25 @@ grub_util_raid_getmembers (const char *name, int bootable)
devicelist = xcalloc (info.nr_disks + 1, sizeof (char *));
- for (i = 0, j = 0; j < info.nr_disks; i++)
+ remaining = info.nr_disks;
+ for (i = 0, j = 0; i < MD_MAX_DISKS && remaining > 0; 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));
-
+
+ /* Skip empty slot: MD_DISK_REMOVED slots don't count toward nr_disks. */
if (disk.state & (1 << MD_DISK_REMOVED))
continue;
+ remaining--;
- if (disk.state & (1 << MD_DISK_ACTIVE))
- devicelist[j] = grub_find_device (NULL,
- makedev (disk.major, disk.minor));
- else
- devicelist[j] = NULL;
- j++;
+ /* Only examine disks that are actively participating in the array. */
+ if (!(disk.state & (1 << MD_DISK_ACTIVE)))
+ continue;
+
+ devicelist[j++] = grub_find_device (NULL, makedev (disk.major,
+ disk.minor));
}
devicelist[j] = NULL;
--
2.25.1
--
Kees Cook
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-01-17 21:38 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-16 17:27 [PATCH] Fix md RAID enumeration Kees Cook
2021-01-17 21:38 ` [PATCH] osdep/linux: Fix md array device enumeration Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.