Re: [PATCH] Generalize MD_MAX_DISKS to GRUB_MDRAID_MAX_DISKS

[Kees Cook]

On Tue, Jun 13, 2023 at 02:54:48PM +0200, Julian Andres Klode wrote:
> Move the constant from getroot.c to disk.h and then reuse
> it place of the hardcoded 1024 limit in diskfilter.
> 
> Fixes: 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more than 
> 1024 disks)
> Cc: Daniel Axtens <dja@axtens.net>
> Cc: Kees Cook <keescook@chromium.org>

Yup, looks good to me. Thanks!

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  grub-core/disk/diskfilter.c     |  4 ++--
>  grub-core/osdep/linux/getroot.c | 12 ++----------
>  include/grub/disk.h             | 10 ++++++++++
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 1c568927b..61a311efd 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -1046,8 +1046,8 @@ grub_diskfilter_make_raid (grub_size_t uuidlen, char 
> *uuid, int nmemb,
>    struct grub_diskfilter_pv *pv;
>    grub_err_t err;
>  
> -  /* We choose not to support more than 1024 disks. */
> -  if (nmemb < 1 || nmemb > 1024)
> +  /* We choose not to support more than the specified number of disks. */
> +  if (nmemb < 1 || nmemb > GRUB_MDRAID_MAX_DISKS)
>      {
>        grub_free (uuid);
>        return NULL;
> diff --git a/grub-core/osdep/linux/getroot.c b/grub-core/osdep/linux/getroot.c
> index 9cf037ec2..7dd775d2a 100644
> --- a/grub-core/osdep/linux/getroot.c
> +++ b/grub-core/osdep/linux/getroot.c
> @@ -44,6 +44,7 @@
>  
>  #include <grub/mm.h>
>  #include <grub/misc.h>
> +#include <grub/disk.h>
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  #include <grub/emu/getroot.h>
> @@ -130,15 +131,6 @@ 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)
>  {
> @@ -176,7 +168,7 @@ grub_util_raid_getmembers (const char *name, int bootable)
>    devicelist = xcalloc (info.nr_disks + 1, sizeof (char *));
>  
>    remaining = info.nr_disks;
> -  for (i = 0, j = 0; i < MD_MAX_DISKS && remaining > 0; i++)
> +  for (i = 0, j = 0; i < GRUB_MDRAID_MAX_DISKS && remaining > 0; i++)
>      {
>        disk.number = i;
>        ret = ioctl (fd, GET_DISK_INFO, &disk);
> diff --git a/include/grub/disk.h b/include/grub/disk.h
> index 071b2f7df..cbd05e9c7 100644
> --- a/include/grub/disk.h
> +++ b/include/grub/disk.h
> @@ -172,6 +172,16 @@ typedef struct grub_disk_memberlist 
> *grub_disk_memberlist_t;
>  /* The maximum number of disk caches.  */
>  #define GRUB_DISK_CACHE_NUM  1021
>  
> +/* The maximum number of disks in an mdraid device.
> + *
> + * 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 GRUB_MDRAID_MAX_DISKS        4096
> +
>  /* The size of a disk cache in 512B units. Must be at least as big as the
>     largest supported sector size, currently 16K.  */
>  #define GRUB_DISK_CACHE_BITS 6
> -- 
> 2.40.1
> 

-- 
Kees Cook