[PATCH] osdep/linux: Fix md array device enumeration

[PATCH] osdep/linux: Fix md array device enumeration

[kees]

From: Kees Cook <kees@ubuntu.com>

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>
---
Hi, this is a resend. Petr reviewed an earlier version back in Jan[1],
but given the changes[2] I didn't want to assume that still stood. :)
Regardless, I'd still like to see this merged so I don't have to
trip over this bug again. ;)
[1] https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00040.html
[2] https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00030.html
---
 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 cd588588eecf..a359d749fef5 100644
--- a/grub-core/osdep/linux/getroot.c
+++ b/grub-core/osdep/linux/getroot.c
@@ -130,10 +130,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;
@@ -165,22 +173,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.30.2

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Daniel Kiper]

On Sat, Sep 25, 2021 at 07:03:35PM -0700, kees@ubuntu.com wrote:
> From: Kees Cook <kees@ubuntu.com>
>
> 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

Please add empty line here.

> Signed-off-by: Kees Cook <kees@ubuntu.com>
> ---
> Hi, this is a resend. Petr reviewed an earlier version back in Jan[1],
> but given the changes[2] I didn't want to assume that still stood. :)
> Regardless, I'd still like to see this merged so I don't have to
> trip over this bug again. ;)
> [1] https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00040.html
> [2] https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00030.html
> ---
>  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 cd588588eecf..a359d749fef5 100644
> --- a/grub-core/osdep/linux/getroot.c
> +++ b/grub-core/osdep/linux/getroot.c
> @@ -130,10 +130,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.  */

Please format comments according to this [1].

> +#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;
> @@ -165,22 +173,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));
> -
> +

I am OK with this change but please mention in the commit message that
you are fixing this on the occasion.

> +      /* 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));

I would prefer if you leave original if statement as is and update
grub_find_device() call accordingly... And of course drop else as
needed... :-)

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Petr Vorel]

Hi Kees, Daniel,

> On Sat, Sep 25, 2021 at 07:03:35PM -0700, kees@ubuntu.com wrote:
> > From: Kees Cook <kees@ubuntu.com>

> > 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).

Kees, thanks a lot for taking care for this.

<snip>
> > +      /* 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));

> I would prefer if you leave original if statement as is and update
> grub_find_device() call accordingly... And of course drop else as
> needed... :-)
I suppose you'll need to send v2 due this, but for now:

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

Kind regards,
Petr

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Kees Cook]

Hi Daniel,

On Tue, Oct 05, 2021 at 06:38:13PM +0200, Daniel Kiper wrote:
> On Sat, Sep 25, 2021 at 07:03:35PM -0700, kees@ubuntu.com wrote:
> > From: Kees Cook <kees@ubuntu.com>
> >
> > 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
> 
> Please add empty line here.

Done!

> 
> > Signed-off-by: Kees Cook <kees@ubuntu.com>
> > ---
> > Hi, this is a resend. Petr reviewed an earlier version back in Jan[1],
> > but given the changes[2] I didn't want to assume that still stood. :)
> > Regardless, I'd still like to see this merged so I don't have to
> > trip over this bug again. ;)
> > [1] https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00040.html
> > [2] https://lists.gnu.org/archive/html/grub-devel/2021-01/msg00030.html
> > ---
> >  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 cd588588eecf..a359d749fef5 100644
> > --- a/grub-core/osdep/linux/getroot.c
> > +++ b/grub-core/osdep/linux/getroot.c
> > @@ -130,10 +130,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.  */
> 
> Please format comments according to this [1].

Sure! I was trying to match the comments already in that file. :)

> 
> > +#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;
> > @@ -165,22 +173,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));
> > -
> > +
> 
> I am OK with this change but please mention in the commit message that
> you are fixing this on the occasion.

Er, okay, yes. It's kind of part of the same overall problem, but I've
tried to specifically call it out in the changelog for v2.

> 
> > +      /* 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));
> 
> I would prefer if you leave original if statement as is and update
> grub_find_device() call accordingly... And of course drop else as
> needed... :-)

Sure!

Thanks for the review; I've sent a v2 now.

-Kees

-- 
Kees Cook

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Kees Cook]

Hi Petr,

On Wed, Oct 06, 2021 at 09:28:32AM +0200, Petr Vorel wrote:
> Hi Kees, Daniel,
> 
> > On Sat, Sep 25, 2021 at 07:03:35PM -0700, kees@ubuntu.com wrote:
> > > From: Kees Cook <kees@ubuntu.com>
> 
> > > 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).
> 
> Kees, thanks a lot for taking care for this.

You bet! I seem to find some amazing corner cases. ;)

> 
> <snip>
> > > +      /* 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));
> 
> > I would prefer if you leave original if statement as is and update
> > grub_find_device() call accordingly... And of course drop else as
> > needed... :-)
> I suppose you'll need to send v2 due this, but for now:
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

Thanks! I've included your reviewed-by in the v2; hopefully that's okay
as the changes were just stylistic.

-Kees

-- 
Kees Cook

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Petr Vorel]

Hi Kees,

> Hi Petr,

> On Wed, Oct 06, 2021 at 09:28:32AM +0200, Petr Vorel wrote:
> > Hi Kees, Daniel,

> > > On Sat, Sep 25, 2021 at 07:03:35PM -0700, kees@ubuntu.com wrote:
> > > > From: Kees Cook <kees@ubuntu.com>

> > > > 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).

> > Kees, thanks a lot for taking care for this.

> You bet! I seem to find some amazing corner cases. ;)
Yep, but both of your bug reports are really worth of fixing.


> > <snip>
> > > > +      /* 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));

> > > I would prefer if you leave original if statement as is and update
> > > grub_find_device() call accordingly... And of course drop else as
> > > needed... :-)
> > I suppose you'll need to send v2 due this, but for now:

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

> Thanks! I've included your reviewed-by in the v2; hopefully that's okay
> as the changes were just stylistic.
Sure, changes in v2 are really minor + already asked to do by Daniel.
Thanks!

Kind regards,
Petr

> -Kees

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Daniel Kiper]

On Wed, Jun 07, 2023 at 02:33:35PM -0700, Kees Cook wrote:
> On Wed, Jun 07, 2023 at 03:39:24PM +0200, Daniel Kiper wrote:
> > On Tue, Jun 06, 2023 at 11:02:31AM -0700, Kees Cook wrote:
> > > On Tue, Jun 6, 2023 at 10:27 AM Julian Andres Klode
> > > <julian.klode@canonical.com> wrote:
> > > >
> > > > On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote:
> > > > > On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> > > > > > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > > > > [...]
> > > > > This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
> > > > > device enumeration).
> > >
> > > Oh good. I really thought it had landed already, so thanks for
> > > checking. I got worried this morning when I saw the email to
> > > grub-devel. :P "Wasn't that fixed already?" :) But thank you for
> > > making sure it hadn't gotten lost! Is there a way to close the tracker
> > > item for it?
> >
> > I think you should be able to do that.
>
> Ah-ha, yes, I've closed it now. :)
> https://salsa.debian.org/grub-team/grub/-/merge_requests/23

Cool! Thanks a lot!

> > > > [...]
> > > > > I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
> > > > > (osdep/linux: Fix md array device enumeration) is not in sync with
> > > > > commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
> > > > > than 1024 disks). I think we should sync both numbers down to 1024...
> > > >
> > > > +1
> > >
> > > Yeah, seems reasonable, though as I hinted in the original patch, this
> > > number appeared to have been arbitrarily chosen by mdadm at the time.
> >
> > OK, we will bump it to 4096 as well.
>
> Yeah, I think _technically_ it can be higher than 1024, though ... I
> struggle to imagine this for a boot device. ;)

Yeah, same here... :-)

Daniel

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Kees Cook]

On Wed, Jun 07, 2023 at 03:39:24PM +0200, Daniel Kiper wrote:
> On Tue, Jun 06, 2023 at 11:02:31AM -0700, Kees Cook wrote:
> > On Tue, Jun 6, 2023 at 10:27 AM Julian Andres Klode
> > <julian.klode@canonical.com> wrote:
> > >
> > > On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote:
> > > > On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> > > > > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > > > [...]
> > > > This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
> > > > device enumeration).
> >
> > Oh good. I really thought it had landed already, so thanks for
> > checking. I got worried this morning when I saw the email to
> > grub-devel. :P "Wasn't that fixed already?" :) But thank you for
> > making sure it hadn't gotten lost! Is there a way to close the tracker
> > item for it?
> 
> I think you should be able to do that.

Ah-ha, yes, I've closed it now. :)
https://salsa.debian.org/grub-team/grub/-/merge_requests/23

> 
> > > [...]
> > > > I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
> > > > (osdep/linux: Fix md array device enumeration) is not in sync with
> > > > commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
> > > > than 1024 disks). I think we should sync both numbers down to 1024...
> > >
> > > +1
> >
> > Yeah, seems reasonable, though as I hinted in the original patch, this
> > number appeared to have been arbitrarily chosen by mdadm at the time.
> 
> OK, we will bump it to 4096 as well.

Yeah, I think _technically_ it can be higher than 1024, though ... I
struggle to imagine this for a boot device. ;)

-- 
Kees Cook

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Daniel Kiper]

On Tue, Jun 06, 2023 at 11:02:31AM -0700, Kees Cook wrote:
> On Tue, Jun 6, 2023 at 10:27 AM Julian Andres Klode
> <julian.klode@canonical.com> wrote:
> >
> > On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote:
> > > On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> > > > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > > [...]
> > > This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
> > > device enumeration).
>
> Oh good. I really thought it had landed already, so thanks for
> checking. I got worried this morning when I saw the email to
> grub-devel. :P "Wasn't that fixed already?" :) But thank you for
> making sure it hadn't gotten lost! Is there a way to close the tracker
> item for it?

I think you should be able to do that. If not I can try to do that for you.

> > [...]
> > > I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
> > > (osdep/linux: Fix md array device enumeration) is not in sync with
> > > commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
> > > than 1024 disks). I think we should sync both numbers down to 1024...
> >
> > +1
>
> Yeah, seems reasonable, though as I hinted in the original patch, this
> number appeared to have been arbitrarily chosen by mdadm at the time.

OK, we will bump it to 4096 as well.

Daniel

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Kees Cook]

On Tue, Jun 6, 2023 at 10:27 AM Julian Andres Klode
<julian.klode@canonical.com> wrote:
>
> On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote:
> > On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> > > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > [...]
> > This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
> > device enumeration).

Oh good. I really thought it had landed already, so thanks for
checking. I got worried this morning when I saw the email to
grub-devel. :P "Wasn't that fixed already?" :) But thank you for
making sure it hadn't gotten lost! Is there a way to close the tracker
item for it?

> [...]
> > I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
> > (osdep/linux: Fix md array device enumeration) is not in sync with
> > commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
> > than 1024 disks). I think we should sync both numbers down to 1024...
>
> +1

Yeah, seems reasonable, though as I hinted in the original patch, this
number appeared to have been arbitrarily chosen by mdadm at the time.

-Kees

-- 
Kees Cook

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Julian Andres Klode]

On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote:
> On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > > From: Kees Cook <keescook@google.com>
> > >
> > > 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>
> >
> > I did not add a cover letter, I just found this patch in a merge request
> > on Debian's GitLab, and it still applies, but I haven't tested it
> > further than Kees did, and don't know what the test case is quite
> > honestly.
> 
> This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
> device enumeration).

Huh sorry,

I must have been looking at a wrong branch and applied to a wrong
branch before forwarding.

> 
> I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
> (osdep/linux: Fix md array device enumeration) is not in sync with
> commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
> than 1024 disks). I think we should sync both numbers down to 1024...

+1

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Daniel Kiper]

On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > From: Kees Cook <keescook@google.com>
> >
> > 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>
>
> I did not add a cover letter, I just found this patch in a merge request
> on Debian's GitLab, and it still applies, but I haven't tested it
> further than Kees did, and don't know what the test case is quite
> honestly.

This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
device enumeration).

I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
(osdep/linux: Fix md array device enumeration) is not in sync with
commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
than 1024 disks). I think we should sync both numbers down to 1024...

Daniel

Re: [PATCH] osdep/linux: Fix md array device enumeration

[Julian Andres Klode]

On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> From: Kees Cook <keescook@google.com>
> 
> 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>

I did not add a cover letter, I just found this patch in a merge request
on Debian's GitLab, and it still applies, but I haven't tested it
further than Kees did, and don't know what the test case is quite
honestly.
-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

[PATCH] osdep/linux: Fix md array device enumeration

[Julian Andres Klode]

From: Kees Cook <keescook@google.com>

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>
---
 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 001b818fe..d7b62e702 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.39.2

[PATCH] osdep/linux: Fix md array device enumeration

[Kees Cook]

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