From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Calvince Otieno <calvncce@gmail.com>
Cc: gustavo@embeddedor.com, outreachy@lists.linux.dev,
linux-staging@lists.linux.dev,
Julia Lawall <julia.lawall@inria.fr>,
Deepak <dvarma04@hotmail.com>
Subject: Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member
Date: Wed, 25 Oct 2023 11:48:16 +0200 [thread overview]
Message-ID: <2023102539-maritime-preamble-1d6b@gregkh> (raw)
In-Reply-To: <CADFX3OTw=6mhEwiM9nA5VZ472J3W+BJQexujr88zEeJDAbPe6A@mail.gmail.com>
On Wed, Oct 25, 2023 at 12:21:05PM +0300, Calvince Otieno wrote:
> On Wed, Oct 25, 2023 at 12:05 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 25, 2023 at 11:58:56AM +0300, Calvince Otieno wrote:
> > > On Wed, Oct 25, 2023 at 11:36 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Oct 25, 2023 at 11:27:06AM +0300, Calvince Otieno wrote:
> > > > > Declaring zero-length arrays is allowed in GNU C as an extension.
> > > > > Although the size of a zero-length array is zero, an array member of
> > > > > this kind may increase the size of the enclosing type as a result of
> > > > > tail padding. The offset of a zero-length array member from the beginning
> > > > > of the enclosing structure is the same as the offset of an array with one
> > > > > or more elements of the same type. The alignment of a zero-length array is
> > > > > the same as the alignment of its elements.
> > > > >
> > > > > Declaring zero-length arrays in other contexts, including as interior
> > > > > members of structure objects or as non-member objects, is discouraged.
> > > > > Accessing elements of zero-length arrays declared in such contexts is
> > > > > undefined and may be diagnosed.
> > > > >
> > > > > There are some instances of code in which the sizeof operator is being
> > > > > incorrectly/erroneously applied to zero-length arrays and the result
> > > > > is zero. Such instances may be hiding some bugs.
> > > > >
> > > > > This issue was found with the help of Coccinelle.
> > > > >
> > > > > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > > > >
> > > > > Signed-off-by: Calvince Otieno <calvncce@gmail.com>
> > > > > ---
> > > > > drivers/staging/wlan-ng/p80211metastruct.h | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/wlan-ng/p80211metastruct.h b/drivers/staging/wlan-ng/p80211metastruct.h
> > > > > index a52217c9b953..c8b73c867391 100644
> > > > > --- a/drivers/staging/wlan-ng/p80211metastruct.h
> > > > > +++ b/drivers/staging/wlan-ng/p80211metastruct.h
> > > > > @@ -71,7 +71,6 @@ struct p80211msg_dot11req_scan_results {
> > > > > struct p80211item_uint32 signal;
> > > > > struct p80211item_uint32 noise;
> > > > > struct p80211item_pstr6 bssid;
> > > > > - u8 pad_3C[1];
> > > >
> > > > But this is not a flexible or 0 length array at all. Why change this?
> > > >
> > > > And are you sure you are allowed to change this? Did you verify where
> > > > this structure is being used and how it is being used and why this
> > > > padding field is in here?
> > > >
> > > > And how was this tested?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > > I have looked through the code to see where the pad_3C member variable
> > > is referenced or used, but I didn't find any instances.
> >
> > I think that is because it is being used to map a structure on top of a
> > data blob read from the device. Dig in and I think you will find where
> > it is mapped somewhere.
> >
> > > I have to admit that my search might not have covered all the possible patterns
> > > and usage scenarios. It appears that the member variable is only declared
> > > within the struct p80211msg_dot11req_scan_results.
> > >
> > > Dan outlines that the pad_3C member variable is used for padding. So,
> > > I stand corrected.
> >
> > I'm interested in what tool told you that this was a variable length
> > array that should be modified? It is a 1 element array, in the middle
> > of a structure, which is not what a variable length array looks like at
> > all, so perhaps some tool needs to be fixed as to not trigger on valid
> > code like this?
> >
> > thanks,
> >
> > greg k-h
>
> Actually, it is a simple coccinelle semantic script of my own making.
> I executed the script against the drivers/staging/wlan-ng
>
> I was trying to match scenarios like the following:
> struct a {
> some var[1]
> }
> struct b{
> some var[0]
> }
> I was reading through the variable length array suggestions and I came
> up with that.
An array of [1] at the _end_ of a structure is a hint that it might be a
variable length array, but I think all of those instances are already
caught in the tree due to the work of others. See the archives of the
linux-hardening mailing list for the work happening there on this, and
for coccinelle scripts to help out if you are interested.
But note, I don't know if this has anything to do with outreachy
application stuff, so don't think I'm asking you to do anything here.
thanks,
greg k-h
prev parent reply other threads:[~2023-10-25 9:48 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-25 8:27 [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member Calvince Otieno
2023-10-25 8:31 ` Dan Carpenter
2023-10-25 8:35 ` Greg Kroah-Hartman
2023-10-25 8:58 ` Calvince Otieno
2023-10-25 9:05 ` Greg Kroah-Hartman
2023-10-25 9:21 ` Calvince Otieno
2023-10-25 9:48 ` Greg Kroah-Hartman [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2023102539-maritime-preamble-1d6b@gregkh \
--to=gregkh@linuxfoundation.org \
--cc=calvncce@gmail.com \
--cc=dvarma04@hotmail.com \
--cc=gustavo@embeddedor.com \
--cc=julia.lawall@inria.fr \
--cc=linux-staging@lists.linux.dev \
--cc=outreachy@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).