[PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Calvince Otieno]

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];
 	struct p80211item_pstr32 ssid;
 	u8 pad_4D[3];
 	struct p80211item_uint32 bsstype;
@@ -95,6 +94,7 @@ struct p80211msg_dot11req_scan_results {
 	struct p80211item_uint32 capinfo;
 	struct p80211item_uint32 basicrate[8];
 	struct p80211item_uint32 supprate[8];
+	u8 pad_3C[];
 } __packed;
 
 struct p80211msg_dot11req_start {
-- 
2.34.1

Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Dan Carpenter]

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];
                  ^
This is not a zero.  This is a one.  It's put there very deliberately to
"pad" the struct.

regards,
dan carpenter
> 

Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Greg Kroah-Hartman]

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

Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Calvince Otieno]

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 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.
Thanks.


-- 
Kind regards,
Calvince Otieno

Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Greg Kroah-Hartman]

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

Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Calvince Otieno]

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.

-- 
Kind regards,
Calvince Otieno

Re: [PATCH] staging: wlan-ng: prism2mgmt.c: rewrite flexible array member

[Greg Kroah-Hartman]

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