[PATCH 1/2] diskfilter: check calloc() result for NULL

[PATCH 1/2] diskfilter: check calloc() result for NULL

[Daniel Axtens]

With wildly corrupt inputs, we can end up trying to calloc a very
large amount of memory, which will fail and give us a NULL pointer.
We need to check that to avoid a crash. (And, even if we blocked
such inputs, it is good practice to check the results of allocations
anyway.)

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 grub-core/disk/diskfilter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
index 2edcff6e8987..4ac50320ef4e 100644
--- a/grub-core/disk/diskfilter.c
+++ b/grub-core/disk/diskfilter.c
@@ -1163,6 +1163,9 @@ grub_diskfilter_make_raid (grub_size_t uuidlen, char *uuid, int nmemb,
   array->lvs->segments->raid_member_size = disk_size;
   array->lvs->segments->nodes
     = grub_calloc (nmemb, sizeof (array->lvs->segments->nodes[0]));
+  if (array->lvs->segments->nodes == NULL)
+    goto fail;
+
   array->lvs->segments->stripe_size = stripe_size;
   for (i = 0; i < nmemb; i++)
     {
-- 
2.25.1

[PATCH 2/2] diskfilter: don't make a RAID array with more than 1024 disks

[Daniel Axtens]

This is 'belt and braces' with the last fix: we end up trying to use
too much memory in situations like corrupted Linux software raid setups
purporting to usew a huge number of disks. Simply refuse to permit such
configurations.

1024 is a bit arbitrary, yes, and I feel a bit like I'm tempting fate
here, but I think 1024 disks in an array (that grub has to read to boot!)
should be enough for anyone.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 grub-core/disk/diskfilter.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
index 4ac50320ef4e..79c5f4db940a 100644
--- a/grub-core/disk/diskfilter.c
+++ b/grub-core/disk/diskfilter.c
@@ -1046,6 +1046,13 @@ 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 > 1024)
+    {
+      grub_free (uuid);
+      return NULL;
+    }
+
   switch (level)
     {
     case 1:
-- 
2.25.1

Re: [PATCH 2/2] diskfilter: don't make a RAID array with more than 1024 disks

[Daniel Axtens]

Daniel Axtens <dja@axtens.net> writes:

> This is 'belt and braces' with the last fix: we end up trying to use
> too much memory in situations like corrupted Linux software raid setups
> purporting to usew a huge number of disks. Simply refuse to permit such
> configurations.
>
> 1024 is a bit arbitrary, yes, and I feel a bit like I'm tempting fate
> here, but I think 1024 disks in an array (that grub has to read to boot!)
> should be enough for anyone.
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  grub-core/disk/diskfilter.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 4ac50320ef4e..79c5f4db940a 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -1046,6 +1046,13 @@ 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 > 1024)

Ergh, nmemb is an int; I will do a v2 that also checks that it's greater
than 0 (or 1, given that it's RAID? I will do some tests.)

-- d

Re: [PATCH 2/2] diskfilter: don't make a RAID array with more than 1024 disks

[Daniel Kiper]

On Tue, Aug 23, 2022 at 01:55:26AM +1000, Daniel Axtens wrote:
> Daniel Axtens <dja@axtens.net> writes:
>
> > This is 'belt and braces' with the last fix: we end up trying to use
> > too much memory in situations like corrupted Linux software raid setups
> > purporting to usew a huge number of disks. Simply refuse to permit such
> > configurations.
> >
> > 1024 is a bit arbitrary, yes, and I feel a bit like I'm tempting fate
> > here, but I think 1024 disks in an array (that grub has to read to boot!)
> > should be enough for anyone.
> >
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > ---
> >  grub-core/disk/diskfilter.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> > index 4ac50320ef4e..79c5f4db940a 100644
> > --- a/grub-core/disk/diskfilter.c
> > +++ b/grub-core/disk/diskfilter.c
> > @@ -1046,6 +1046,13 @@ 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 > 1024)
>
> Ergh, nmemb is an int; I will do a v2 that also checks that it's greater
> than 0 (or 1, given that it's RAID? I will do some tests.)

Could you repost this patch with the fix? Or both with my RB for first one...

Daniel

Re: [PATCH 1/2] diskfilter: check calloc() result for NULL

[Daniel Kiper]

On Sun, Aug 21, 2022 at 10:22:35PM +1000, Daniel Axtens wrote:
> With wildly corrupt inputs, we can end up trying to calloc a very
> large amount of memory, which will fail and give us a NULL pointer.
> We need to check that to avoid a crash. (And, even if we blocked
> such inputs, it is good practice to check the results of allocations
> anyway.)
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel