Re: [PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Sat, 7 Oct 2023 21:26:22 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Thu, Sep 28, 2023 at 5:42 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Wed, 27 Sep 2023 10:52:18 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports
> > > error: super-ddf.c:1988:58: error: array subscript -1 is below array
> > > bounds of ‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]
> > > The subscrit is defined as int type. And it can be smaller than 0.  
> >
> > If it can be smaller that 0 then it is something we need to fix.
> > I think that it comes from here:
> >                 info->disk.raid_disk = find_phys(ddf,
> > ddf->dlist->disk.refnum); info->data_offset = be64_to_cpu(ddf->phys->
> >                                                   entries[info->disk.raid_disk].
> >                                                   config_size);
> >
> > find_phys can return -1.
> > It is handled few lines bellow. I don't see reason why we cannot handle it
> > here too.
> >
> >                 if (info->disk.raid_disk >= 0)
> >                         pde = ddf->phys->entries + info->disk.raid_disk;
> >
> > I think that it will be fair to abort because metadata seems to be
> > corrupted. We are referring to info->disk.raid_disk from many places. We
> > cannot return error because it is void, we can just return.  
> 
> Hi Mariusz
> 
> You mean something like this?
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index 7213284e0a59..b6e514042055 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -1984,6 +1984,9 @@ static void getinfo_super_ddf(struct supertype
> *st, struct mdinfo *info, char *m
>                 info->disk.number = be32_to_cpu(ddf->dlist->disk.refnum);
>                 info->disk.raid_disk = find_phys(ddf,
> ddf->dlist->disk.refnum);
> 
> +               if (info->disk.raid_disk < 0)
> +                       return;
> +
>                 info->data_offset = be64_to_cpu(ddf->phys->
>                                                   entries[info->disk.raid_disk].
>                                                   config_size);
> 

Yes, LGTM!

Mariusz