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