diff mbox series

staging: r8188eu: Use flexible-array for one length array member

Message ID Y1vRivFfRD6VoBt/@ubunlion
State New
Headers
Series staging: r8188eu: Use flexible-array for one length array member |

Commit Message

Deepak R Varma Oct. 28, 2022, 12:56 p.m. UTC 
  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@mailo.com>
---
 drivers/staging/r8188eu/include/odm.h         | 2 +-
 drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

--
2.34.1

Comments

Pavel Skripkin Oct. 28, 2022, 2:03 p.m. UTC | #1 
Hi Deepak R,

Deepak R Varma <drv@mailo.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@mailo.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

>   struct odm_rf_cal {
> diff --git a/drivers/staging/r8188eu/include/wlan_bssdef.h b/drivers/staging/r8188eu/include/wlan_bssdef.h
> index 831c465df500..33177de194eb 100644
> --- a/drivers/staging/r8188eu/include/wlan_bssdef.h
> +++ b/drivers/staging/r8188eu/include/wlan_bssdef.h
> @@ -179,7 +179,7 @@ struct ndis_802_11_status_ind {
> 
>   struct ndis_802_11_auth_evt {
>   	struct ndis_802_11_status_ind       Status;
> -	struct ndis_802_11_auth_req  Request[1];
> +	struct ndis_802_11_auth_req  Request[];
>   };
> 

this structure seems to be unused. Better to remove it instead of 
maintaining the old code

>   struct ndis_802_11_test {
> @@ -291,7 +291,7 @@ struct pmkid_candidate {
>   struct ndis_802_11_pmkid_list {
>   	u32 Version;       /*  Version of the structure */
>   	u32 NumCandidates; /*  No. of pmkid candidates */
> -	struct pmkid_candidate CandidateList[1];
> +	struct pmkid_candidate CandidateList[];
>   };

this one as well

> 
>   struct ndis_802_11_auth_encrypt {
> @@ -304,7 +304,7 @@ struct ndis_802_11_cap {
>   	u32  Version;
>   	u32  NoOfPMKIDs;
>   	u32  NoOfAuthEncryptPairsSupported;
> -	struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[1];
> +	struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[];
>   };
> 
>   u8 key_2char2num(u8 hch, u8 lch);
> --
> 2.34.1
> 

and this one as well

> 
> 





With regards,
Pavel Skripkin
Deepak R Varma Oct. 28, 2022, 2:11 p.m. UTC | #2 
On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> Hi Deepak R,
>
> Deepak R Varma <drv@mailo.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@mailo.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?

>
> >   struct odm_rf_cal {
> > diff --git a/drivers/staging/r8188eu/include/wlan_bssdef.h b/drivers/staging/r8188eu/include/wlan_bssdef.h
> > index 831c465df500..33177de194eb 100644
> > --- a/drivers/staging/r8188eu/include/wlan_bssdef.h
> > +++ b/drivers/staging/r8188eu/include/wlan_bssdef.h
> > @@ -179,7 +179,7 @@ struct ndis_802_11_status_ind {
> >
> >   struct ndis_802_11_auth_evt {
> >   	struct ndis_802_11_status_ind       Status;
> > -	struct ndis_802_11_auth_req  Request[1];
> > +	struct ndis_802_11_auth_req  Request[];
> >   };
> >
>
> this structure seems to be unused. Better to remove it instead of
> maintaining the old code

Sounds good. I agree and will do as suggested.

>
> >   struct ndis_802_11_test {
> > @@ -291,7 +291,7 @@ struct pmkid_candidate {
> >   struct ndis_802_11_pmkid_list {
> >   	u32 Version;       /*  Version of the structure */
> >   	u32 NumCandidates; /*  No. of pmkid candidates */
> > -	struct pmkid_candidate CandidateList[1];
> > +	struct pmkid_candidate CandidateList[];
> >   };
>
> this one as well

Yes, same here.

>
> >
> >   struct ndis_802_11_auth_encrypt {
> > @@ -304,7 +304,7 @@ struct ndis_802_11_cap {
> >   	u32  Version;
> >   	u32  NoOfPMKIDs;
> >   	u32  NoOfAuthEncryptPairsSupported;
> > -	struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[1];
> > +	struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[];
> >   };
> >
> >   u8 key_2char2num(u8 hch, u8 lch);
> > --
> > 2.34.1
> >
>
> and this one as well

Yes, will do.

Thank you again for your feedback and suggestions.

./drv

>
> >
> >
>
>
>
>
>
> With regards,
> Pavel Skripkin
>
Dan Carpenter Oct. 28, 2022, 2:43 p.m. UTC | #3 
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@mailo.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@mailo.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
Deepak R Varma Nov. 2, 2022, 7:19 p.m. UTC | #4 
On Fri, Oct 28, 2022 at 05:43:03PM +0300, Dan Carpenter wrote:
> 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@mailo.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@mailo.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.

Hello Pavel and Dan,
I looked at the surround code and places where the proposed patch will have an
impact. As you rightly pointed out this array can't be treated as a flexible
array since it fits right in the middle of another struct.

I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
array, why is it declared as a two dimensional array? Can this be a simple one
dimensional array instead? Is this the cleanup you are referring to when you say
"code is messy and should be cleaned up"?


Thank you,
./drv

>
> regards,
> dan carpenter
>
Dan Carpenter Nov. 3, 2022, 5:22 a.m. UTC | #5 
On Thu, Nov 03, 2022 at 12:49:38AM +0530, Deepak R Varma wrote:
> I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
> array, why is it declared as a two dimensional array? Can this be a simple one
> dimensional array instead? Is this the cleanup you are referring to when you say
> "code is messy and should be cleaned up"?

Yes.  You have interpreted that correctly.

regards,
dan carpenter
Deepak R Varma Nov. 3, 2022, 5:25 a.m. UTC | #6 
On Thu, Nov 03, 2022 at 08:22:14AM +0300, Dan Carpenter wrote:
> On Thu, Nov 03, 2022 at 12:49:38AM +0530, Deepak R Varma wrote:
> > I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
> > array, why is it declared as a two dimensional array? Can this be a simple one
> > dimensional array instead? Is this the cleanup you are referring to when you say
> > "code is messy and should be cleaned up"?
>
> Yes.  You have interpreted that correctly.

Thank you for the confirmation. This will be an interesting correction. I will
review and submit a patch shortly.

Thank you,
./drv

>
> regards,
> dan carpenter
>
>
diff mbox series

Patch

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];
 };

 struct odm_rf_cal {
diff --git a/drivers/staging/r8188eu/include/wlan_bssdef.h b/drivers/staging/r8188eu/include/wlan_bssdef.h
index 831c465df500..33177de194eb 100644
--- a/drivers/staging/r8188eu/include/wlan_bssdef.h
+++ b/drivers/staging/r8188eu/include/wlan_bssdef.h
@@ -179,7 +179,7 @@  struct ndis_802_11_status_ind {

 struct ndis_802_11_auth_evt {
 	struct ndis_802_11_status_ind       Status;
-	struct ndis_802_11_auth_req  Request[1];
+	struct ndis_802_11_auth_req  Request[];
 };

 struct ndis_802_11_test {
@@ -291,7 +291,7 @@  struct pmkid_candidate {
 struct ndis_802_11_pmkid_list {
 	u32 Version;       /*  Version of the structure */
 	u32 NumCandidates; /*  No. of pmkid candidates */
-	struct pmkid_candidate CandidateList[1];
+	struct pmkid_candidate CandidateList[];
 };

 struct ndis_802_11_auth_encrypt {
@@ -304,7 +304,7 @@  struct ndis_802_11_cap {
 	u32  Version;
 	u32  NoOfPMKIDs;
 	u32  NoOfAuthEncryptPairsSupported;
-	struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[1];
+	struct ndis_802_11_auth_encrypt AuthenticationEncryptionSupported[];
 };

 u8 key_2char2num(u8 hch, u8 lch);