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 <[email protected]>
---
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];
};
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);
--
2.34.1
Hi Deepak R,
Deepak R Varma <[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> Hi Deepak R,
>
> Deepak R Varma <[email protected]> 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 <[email protected]>
> > ---
> > 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
>
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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
>
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
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
>
>