On Sat, Sep 25, 2021 at 07:03:35PM -0700, k...@ubuntu.com wrote:
> From: Kees Cook <k...@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 <k...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
