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