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