linux-kernel - Re: [PATCH] staging: r8188eu: Use flexible-array for one length array member

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y1vqd/ij5WqcL3Qe@kadam>
Date:   Fri, 28 Oct 2022 17:43:03 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Deepak R Varma <drv@...lo.com>
Cc:     Pavel Skripkin <paskripkin@...il.com>, outreachy@...ts.linux.dev,
        Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: r8188eu: Use flexible-array for one length
 array member

On Fri, Oct 28, 2022 at 07:41:54PM +0530, Deepak R Varma wrote:
> On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> > Hi Deepak R,
> >
> > Deepak R Varma <drv@...lo.com> says:
> > > Flexible-array member should be used instead of one or zero member to
> > > meet the need for having a dynamically sized trailing elements in a
> > > structure. Refer to links [1] and [2] for detailed guidance on this
> > > suggestion.
> > >
> > > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> > >
> > > Issue identified using coccicheck.
> > >
> > > Signed-off-by: Deepak R Varma <drv@...lo.com>
> > > ---
> > >   drivers/staging/r8188eu/include/odm.h         | 2 +-
> > >   drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
> > >   2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
> > > index 89b01dd614ba..e2a9de5b9323 100644
> > > --- a/drivers/staging/r8188eu/include/odm.h
> > > +++ b/drivers/staging/r8188eu/include/odm.h
> > > @@ -166,7 +166,7 @@ struct odm_ra_info {
> > >
> > >   struct ijk_matrix_regs_set {
> > >   	bool	bIQKDone;
> > > -	s32	Value[1][IQK_Matrix_REG_NUM];
> > > +	s32	Value[][IQK_Matrix_REG_NUM];
> > >   };
> > >
> >
> > you are changing the actual size of the struct. Wondering if you have tested
> > this patch somehow
> 
> Hello Pavel,
> Thank you for reviewing the patch. I build the module post making the changes an
> ensured that the build is successful. However, I am not sure how to check the
> changes I am proposing. Could you please direct me to some information on how to
> test patches? Is there some documentation generic/driver-specific that I can
> refer to?

You just have to look at every place where it is used and especially
look at where it is allocated.  It is only used in one place:

	struct ijk_matrix_regs_set IQKMatrixRegSetting;

But that is in the middle of a struct and generally it doesn't make
sense to have a flex array in the middle of a struct.  So investigating
further, we see that it really is a one element array:

Do a grep:

	pDM_Odm->RFCalibrateInfo.IQKMatrixRegSetting[ChannelMappedIndex].Value[0][1]);

The first element is always zero.  So this patch introduces memory
corruption.

The code is messy and should be cleaned up, of course.

regards,
dan carpenter