linux-kernel - Re: [PATCH] staging: wlan-ng: Use flexible-array for one / zero-length arrays

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: <Y1vjsTMsvVFuMIYb@kadam>
Date:   Fri, 28 Oct 2022 17:14:09 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Deepak R Varma <drv@...lo.com>
Cc:     outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: wlan-ng: Use flexible-array for one /
 zero-length arrays

On Fri, Oct 28, 2022 at 07:11:00PM +0530, Deepak R Varma wrote:
> On Fri, Oct 28, 2022 at 04:33:59PM +0300, Dan Carpenter wrote:
> > On Fri, Oct 28, 2022 at 06:08:13PM +0530, Deepak R Varma wrote:
> > > 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/wlan-ng/p80211mgmt.h  | 8 ++++----
> > >  drivers/staging/wlan-ng/p80211types.h | 2 +-
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/wlan-ng/p80211mgmt.h b/drivers/staging/wlan-ng/p80211mgmt.h
> > > index 1ef30d3f3159..d6fe52de2c8f 100644
> > > --- a/drivers/staging/wlan-ng/p80211mgmt.h
> > > +++ b/drivers/staging/wlan-ng/p80211mgmt.h
> > > @@ -229,14 +229,14 @@ struct wlan_ie {
> > >  struct wlan_ie_ssid {
> > >  	u8 eid;
> > >  	u8 len;
> > > -	u8 ssid[1];		/* may be zero, ptrs may overlap */
> > > +	u8 ssid[];		/* may be zero, ptrs may overlap */
> >
> > How have you ensured that changing this from a 1 byte array to a zero
> > byte array does not break anything?  It's not uncommon for a people
> > to do math like "size - 1 + length".  The "- 1" would be to take the
> > 1 element into consideration.
> 
> Hi Dan,
> I did a code review to understand how this structure member is used and did not
> find any computations you mentioned. I would certainly like to receive your feedback
> as well.
> 

That would be useful information for the commit message.  Even if it
goes under the --- cut off.

---
Compile tested only.  I audited all fourty two uses of the wlan_ie_ssid
struct and nothing checks sizeof(struct wlan_ie_ssid) and changing this
from 1 to 0 will not break anything.

> >
> > I was trying to read through this code to check your work, but then
> > you sent a second patch which also does not explain how you are auditing
> > your changes.  Can you go a bit slower?
> 
> My apologies for rushing patches in. I will hold on for feedback on these
> patches before turning in any new patch involving similar change. I hope it is
> okay to send a different type of patch though. Please correct if I am wrong.

I'm not saying you have to wait for days for reviewers to get through
your patches...  I guess what I'm really trying to say is please group
your patches together.  Pavel Skripkin's review comments seem reasonable
so if the patches are grouped then he only has to reply once but now
someone has to reply to every email.

Also if you write 10 patches maybe you will notice or think of something
part way through and which forces you to re-write the earlier patches.

When you send patches every ten minutes then everyone can see you have
not tested it at all.  I don't really expect people to test their
staging patches, but let's not make it obvious.  Wait overnight so
people think "Ah, maybe they were tested".  It gives people (false)
confidence that this is quality work.

regards,
dan carpenter