[PATCH 0/6][next] ath6kl: wmi: Replace one-element arrays with flexible-array members

[PATCH 0/6][next] ath6kl: wmi: Replace one-element arrays with flexible-array members

[Gustavo A. R. Silva]

This series aims to replace one-element arrays with flexible-array
members in multiple structures in drivers/net/wireless/ath/ath6kl/wmi.h

There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

This helps with the ongoing efforts to globally enable -Warray-bounds
and get us closer to being able to tighten the FORTIFY_SOURCE routines
on memcpy().

These issues were found with the help of Coccinelle and audited and fixed,
manually.

[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

Link: https://github.com/KSPP/linux/issues/79

Gustavo A. R. Silva (6):
  ath6kl: wmi: Replace one-element array with flexible-array member in
    struct wmi_begin_scan_cmd
  ath6kl: wmi: Replace one-element array with flexible-array member in
    struct wmi_start_scan_cmd
  ath6kl: wmi: Replace one-element array with flexible-array  member in
    struct wmi_channel_list_reply
  ath6kl: wmi: Replace one-element array with flexible-array  member in
    struct wmi_connect_event
  ath6kl: wmi: Replace one-element array with flexible-array  member in
    struct wmi_disconnect_event
  ath6kl: wmi: Replace one-element array with flexible-array  member in
    struct wmi_aplist_event

 drivers/net/wireless/ath/ath6kl/wmi.c | 30 +++++++--------------------
 drivers/net/wireless/ath/ath6kl/wmi.h | 12 +++++------
 2 files changed, 14 insertions(+), 28 deletions(-)

-- 
2.27.0

[PATCH 1/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_begin_scan_cmd

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
wmi_begin_scan_cmd. Also, make use of the struct_size() helper.

This issue was found with the help of Coccinelle and audited and fixed,
manually.

Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/wireless/ath/ath6kl/wmi.c | 9 ++-------
 drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index bd1ef6334997..e1c950014f3e 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -2008,7 +2008,7 @@ int ath6kl_wmi_beginscan_cmd(struct wmi *wmi, u8 if_idx,
 	struct ieee80211_supported_band *sband;
 	struct sk_buff *skb;
 	struct wmi_begin_scan_cmd *sc;
-	s8 size, *supp_rates;
+	s8 *supp_rates;
 	int i, band, ret;
 	struct ath6kl *ar = wmi->parent_dev;
 	int num_rates;
@@ -2023,18 +2023,13 @@ int ath6kl_wmi_beginscan_cmd(struct wmi *wmi, u8 if_idx,
 						num_chan, ch_list);
 	}
 
-	size = sizeof(struct wmi_begin_scan_cmd);
-
 	if ((scan_type != WMI_LONG_SCAN) && (scan_type != WMI_SHORT_SCAN))
 		return -EINVAL;
 
 	if (num_chan > WMI_MAX_CHANNELS)
 		return -EINVAL;
 
-	if (num_chan)
-		size += sizeof(u16) * (num_chan - 1);
-
-	skb = ath6kl_wmi_get_new_buf(size);
+	skb = ath6kl_wmi_get_new_buf(struct_size(sc, ch_list, num_chan));
 	if (!skb)
 		return -ENOMEM;
 
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index 784940ba4c90..322539ed9c12 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -863,7 +863,7 @@ struct wmi_begin_scan_cmd {
 	u8 num_ch;
 
 	/* channels in Mhz */
-	__le16 ch_list[1];
+	__le16 ch_list[];
 } __packed;
 
 /* wmi_start_scan_cmd is to be deprecated. Use
-- 
2.27.0

Re: [PATCH 1/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_begin_scan_cmd

[Jeff Johnson]

On 2/22/2022 6:38 PM, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> wmi_begin_scan_cmd. Also, make use of the struct_size() helper.
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>   drivers/net/wireless/ath/ath6kl/wmi.c | 9 ++-------
>   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
>   2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index bd1ef6334997..e1c950014f3e 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -2008,7 +2008,7 @@ int ath6kl_wmi_beginscan_cmd(struct wmi *wmi, u8 if_idx,
>   	struct ieee80211_supported_band *sband;
>   	struct sk_buff *skb;
>   	struct wmi_begin_scan_cmd *sc;
> -	s8 size, *supp_rates;
> +	s8 *supp_rates;
>   	int i, band, ret;
>   	struct ath6kl *ar = wmi->parent_dev;
>   	int num_rates;
> @@ -2023,18 +2023,13 @@ int ath6kl_wmi_beginscan_cmd(struct wmi *wmi, u8 if_idx,
>   						num_chan, ch_list);
>   	}
>   
> -	size = sizeof(struct wmi_begin_scan_cmd);
> -
>   	if ((scan_type != WMI_LONG_SCAN) && (scan_type != WMI_SHORT_SCAN))
>   		return -EINVAL;
>   
>   	if (num_chan > WMI_MAX_CHANNELS)
>   		return -EINVAL;
>   
> -	if (num_chan)
> -		size += sizeof(u16) * (num_chan - 1);
> -
> -	skb = ath6kl_wmi_get_new_buf(size);
> +	skb = ath6kl_wmi_get_new_buf(struct_size(sc, ch_list, num_chan));
>   	if (!skb)
>   		return -ENOMEM;
>   
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> index 784940ba4c90..322539ed9c12 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -863,7 +863,7 @@ struct wmi_begin_scan_cmd {
>   	u8 num_ch;
>   
>   	/* channels in Mhz */
> -	__le16 ch_list[1];
> +	__le16 ch_list[];
>   } __packed;
>   
>   /* wmi_start_scan_cmd is to be deprecated. Use



Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

[PATCH 2/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_start_scan_cmd

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
wmi_start_scan_cmd. Also, make use of the struct_size() helper.

This issue was found with the help of Coccinelle and audited and fixed,
manually.

Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/wireless/ath/ath6kl/wmi.c | 8 +-------
 drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index e1c950014f3e..bdfc057c5a82 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1959,21 +1959,15 @@ static int ath6kl_wmi_startscan_cmd(struct wmi *wmi, u8 if_idx,
 {
 	struct sk_buff *skb;
 	struct wmi_start_scan_cmd *sc;
-	s8 size;
 	int i, ret;
 
-	size = sizeof(struct wmi_start_scan_cmd);
-
 	if ((scan_type != WMI_LONG_SCAN) && (scan_type != WMI_SHORT_SCAN))
 		return -EINVAL;
 
 	if (num_chan > WMI_MAX_CHANNELS)
 		return -EINVAL;
 
-	if (num_chan)
-		size += sizeof(u16) * (num_chan - 1);
-
-	skb = ath6kl_wmi_get_new_buf(size);
+	skb = ath6kl_wmi_get_new_buf(struct_size(sc, ch_list, num_chan));
 	if (!skb)
 		return -ENOMEM;
 
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index 322539ed9c12..9e168752bec2 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -889,7 +889,7 @@ struct wmi_start_scan_cmd {
 	u8 num_ch;
 
 	/* channels in Mhz */
-	__le16 ch_list[1];
+	__le16 ch_list[];
 } __packed;
 
 /*
-- 
2.27.0

[PATCH 3/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_channel_list_reply

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
wmi_channel_list_reply.

It's also worth noting that due to the flexible array transformation,
the size of struct wmi_channel_list_reply changed, see below.

Before flex-array transformation:

struct wmi_channel_list_reply {
	u8                         reserved;             /*     0     1 */
	u8                         num_ch;               /*     1     1 */
	__le16                     ch_list[1];           /*     2     2 */

	/* size: 4, cachelines: 1, members: 3 */
	/* last cacheline: 4 bytes */
};

After flex-array transformation:

struct wmi_channel_list_reply {
	u8                         reserved;             /*     0     1 */
	u8                         num_ch;               /*     1     1 */
	__le16                     ch_list[];            /*     2     0 */

	/* size: 2, cachelines: 1, members: 3 */
	/* last cacheline: 2 bytes */
};

So, the following change preserves the logic that if _len_ is at least
4 bytes in size, this is the existence of at least one channel in
ch_list[] is being considered, then the execution jumps to call
ath6kl_wakeup_event(wmi->parent_dev);, otherwise _len_ is 2 bytes or
less and the code returns -EINVAL:

	-       if (len < sizeof(struct wmi_channel_list_reply))
	+       if (len <= sizeof(struct wmi_channel_list_reply))

This issue was found with the help of Coccinelle and audited and fixed,
manually.

Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Hi!

It'd be great if someone can confirm or comment on the following
changes described in the changelog text:

	-       if (len < sizeof(struct wmi_channel_list_reply))
	+       if (len <= sizeof(struct wmi_channel_list_reply))

Thanks

 drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
 drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index bdfc057c5a82..049d75f31f3c 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1240,7 +1240,7 @@ static int ath6kl_wmi_ratemask_reply_rx(struct wmi *wmi, u8 *datap, int len)
 
 static int ath6kl_wmi_ch_list_reply_rx(struct wmi *wmi, u8 *datap, int len)
 {
-	if (len < sizeof(struct wmi_channel_list_reply))
+	if (len <= sizeof(struct wmi_channel_list_reply))
 		return -EINVAL;
 
 	ath6kl_wakeup_event(wmi->parent_dev);
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index 9e168752bec2..432e4f428a4a 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -1373,7 +1373,7 @@ struct wmi_channel_list_reply {
 	u8 num_ch;
 
 	/* channel in Mhz */
-	__le16 ch_list[1];
+	__le16 ch_list[];
 } __packed;
 
 /* List of Events (target to host) */
-- 
2.27.0

Re: [PATCH 3/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_channel_list_reply

[Jeff Johnson]

On 2/22/2022 6:38 PM, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> wmi_channel_list_reply.
> 
> It's also worth noting that due to the flexible array transformation,
> the size of struct wmi_channel_list_reply changed, see below.
> 
> Before flex-array transformation:
> 
> struct wmi_channel_list_reply {
> 	u8                         reserved;             /*     0     1 */
> 	u8                         num_ch;               /*     1     1 */
> 	__le16                     ch_list[1];           /*     2     2 */
> 
> 	/* size: 4, cachelines: 1, members: 3 */
> 	/* last cacheline: 4 bytes */
> };
> 
> After flex-array transformation:
> 
> struct wmi_channel_list_reply {
> 	u8                         reserved;             /*     0     1 */
> 	u8                         num_ch;               /*     1     1 */
> 	__le16                     ch_list[];            /*     2     0 */
> 
> 	/* size: 2, cachelines: 1, members: 3 */
> 	/* last cacheline: 2 bytes */
> };
> 
> So, the following change preserves the logic that if _len_ is at least
> 4 bytes in size, this is the existence of at least one channel in
> ch_list[] is being considered, then the execution jumps to call
> ath6kl_wakeup_event(wmi->parent_dev);, otherwise _len_ is 2 bytes or
> less and the code returns -EINVAL:
> 
> 	-       if (len < sizeof(struct wmi_channel_list_reply))
> 	+       if (len <= sizeof(struct wmi_channel_list_reply))
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Hi!
> 
> It'd be great if someone can confirm or comment on the following
> changes described in the changelog text:
> 
> 	-       if (len < sizeof(struct wmi_channel_list_reply))
> 	+       if (len <= sizeof(struct wmi_channel_list_reply))

My opinion is this can remain unchanged since being unchanged would 
correctly handle a channel list with no channels whereas the original 
code required that at least one channel be present.

The test is really there just to make sure the entirety of the "fixed" 
portion of the message is present.

Ultimately it doesn't matter since no actual processing of the channel 
list takes place.

If actual processing took place, then it would make sense to have an 
additional test to verify the len is large enough to handle num_ch entries.


> 
> Thanks
> 
>   drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
>   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index bdfc057c5a82..049d75f31f3c 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -1240,7 +1240,7 @@ static int ath6kl_wmi_ratemask_reply_rx(struct wmi *wmi, u8 *datap, int len)
>   
>   static int ath6kl_wmi_ch_list_reply_rx(struct wmi *wmi, u8 *datap, int len)
>   {
> -	if (len < sizeof(struct wmi_channel_list_reply))
> +	if (len <= sizeof(struct wmi_channel_list_reply))
>   		return -EINVAL;
>   
>   	ath6kl_wakeup_event(wmi->parent_dev);
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> index 9e168752bec2..432e4f428a4a 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -1373,7 +1373,7 @@ struct wmi_channel_list_reply {
>   	u8 num_ch;
>   
>   	/* channel in Mhz */
> -	__le16 ch_list[1];
> +	__le16 ch_list[];
>   } __packed;
>   
>   /* List of Events (target to host) */

whether or not you modify the length check consider this:
Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

[PATCH 4/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_connect_event

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
wmi_connect_event.

It's also worth noting that due to the flexible array transformation,
the size of struct wmi_connect_event changed (now the size is 1 byte
smaller), and in order to preserve the logic of before the transformation,
the following change is needed:

	-       if (len < sizeof(struct wmi_connect_event))
	+       if (len <= sizeof(struct wmi_connect_event))

This issue was found with the help of Coccinelle and audited and fixed,
manually.

Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Hi!

It'd be great if someone can confirm or comment on the following
changes described in the changelog text:

        -       if (len < sizeof(struct wmi_connect_event))
        +       if (len <= sizeof(struct wmi_connect_event))

Thanks

 drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
 drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index 049d75f31f3c..ccdccead688e 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -857,7 +857,7 @@ static int ath6kl_wmi_connect_event_rx(struct wmi *wmi, u8 *datap, int len,
 	struct wmi_connect_event *ev;
 	u8 *pie, *peie;
 
-	if (len < sizeof(struct wmi_connect_event))
+	if (len <= sizeof(struct wmi_connect_event))
 		return -EINVAL;
 
 	ev = (struct wmi_connect_event *) datap;
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index 432e4f428a4a..6b064e669d87 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -1545,7 +1545,7 @@ struct wmi_connect_event {
 	u8 beacon_ie_len;
 	u8 assoc_req_len;
 	u8 assoc_resp_len;
-	u8 assoc_info[1];
+	u8 assoc_info[];
 } __packed;
 
 /* Disconnect Event */
-- 
2.27.0

Re: [PATCH 4/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_connect_event

[Jeff Johnson]

On 2/22/2022 6:38 PM, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> wmi_connect_event.
> 
> It's also worth noting that due to the flexible array transformation,
> the size of struct wmi_connect_event changed (now the size is 1 byte
> smaller), and in order to preserve the logic of before the transformation,
> the following change is needed:
> 
> 	-       if (len < sizeof(struct wmi_connect_event))
> 	+       if (len <= sizeof(struct wmi_connect_event))
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Hi!
> 
> It'd be great if someone can confirm or comment on the following
> changes described in the changelog text:
> 
>          -       if (len < sizeof(struct wmi_connect_event))
>          +       if (len <= sizeof(struct wmi_connect_event))
> 
> Thanks
> 
>   drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
>   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index 049d75f31f3c..ccdccead688e 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -857,7 +857,7 @@ static int ath6kl_wmi_connect_event_rx(struct wmi *wmi, u8 *datap, int len,
>   	struct wmi_connect_event *ev;
>   	u8 *pie, *peie;
>   
> -	if (len < sizeof(struct wmi_connect_event))
> +	if (len <= sizeof(struct wmi_connect_event))

this is another case where IMO the original code can remain since all it 
is really checking is to see if the entire "fixed" portion is present. 
in reality if there was just one byte of assoc_info the response would 
actually be invalid.

what is missing is logic that verifies len is large enough to hold the 
payload that is advertised via the beacon_ie_len, assoc_req_len, & 
assoc_resp_len members. without this even if you change the initial len 
check you can have a condition where len says there is one u8 in 
assoc_info (and pass the initial test) but the other three members 
indicate that much more data is present.

but that is a pre-existing shortcoming that should be handled with a 
separate patch.


>   		return -EINVAL;
>   
>   	ev = (struct wmi_connect_event *) datap;
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> index 432e4f428a4a..6b064e669d87 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -1545,7 +1545,7 @@ struct wmi_connect_event {
>   	u8 beacon_ie_len;
>   	u8 assoc_req_len;
>   	u8 assoc_resp_len;
> -	u8 assoc_info[1];
> +	u8 assoc_info[];
>   } __packed;
>   
>   /* Disconnect Event */

whether or not you modify the length check consider this:
Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH 4/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_connect_event

[Gustavo A. R. Silva]

On Wed, Feb 23, 2022 at 04:50:18PM -0800, Jeff Johnson wrote:
> On 2/22/2022 6:38 PM, Gustavo A. R. Silva wrote:
> > Replace one-element array with flexible-array member in struct
> > wmi_connect_event.
> > 
> > It's also worth noting that due to the flexible array transformation,
> > the size of struct wmi_connect_event changed (now the size is 1 byte
> > smaller), and in order to preserve the logic of before the transformation,
> > the following change is needed:
> > 
> > 	-       if (len < sizeof(struct wmi_connect_event))
> > 	+       if (len <= sizeof(struct wmi_connect_event))
> > 
> > This issue was found with the help of Coccinelle and audited and fixed,
> > manually.
> > 
> > Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> > Link: https://github.com/KSPP/linux/issues/79
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> > Hi!
> > 
> > It'd be great if someone can confirm or comment on the following
> > changes described in the changelog text:
> > 
> >          -       if (len < sizeof(struct wmi_connect_event))
> >          +       if (len <= sizeof(struct wmi_connect_event))
> > 
> > Thanks
> > 
> >   drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
> >   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> > index 049d75f31f3c..ccdccead688e 100644
> > --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> > +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> > @@ -857,7 +857,7 @@ static int ath6kl_wmi_connect_event_rx(struct wmi *wmi, u8 *datap, int len,
> >   	struct wmi_connect_event *ev;
> >   	u8 *pie, *peie;
> > -	if (len < sizeof(struct wmi_connect_event))
> > +	if (len <= sizeof(struct wmi_connect_event))
> 
> this is another case where IMO the original code can remain since all it is
> really checking is to see if the entire "fixed" portion is present. in
> reality if there was just one byte of assoc_info the response would actually
> be invalid.

I see; that's actually what I needed to be clarified. I wasn't sure if
a channel list with no channels was valid and expected. Now I see it is. :)

> what is missing is logic that verifies len is large enough to hold the
> payload that is advertised via the beacon_ie_len, assoc_req_len, &
> assoc_resp_len members. without this even if you change the initial len
> check you can have a condition where len says there is one u8 in assoc_info
> (and pass the initial test) but the other three members indicate that much
> more data is present.
> 
> but that is a pre-existing shortcoming that should be handled with a
> separate patch.

Yeah; I'll consider extending this series to include a patch for that.

> >   		return -EINVAL;
> >   	ev = (struct wmi_connect_event *) datap;
> > diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> > index 432e4f428a4a..6b064e669d87 100644
> > --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> > +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> > @@ -1545,7 +1545,7 @@ struct wmi_connect_event {
> >   	u8 beacon_ie_len;
> >   	u8 assoc_req_len;
> >   	u8 assoc_resp_len;
> > -	u8 assoc_info[1];
> > +	u8 assoc_info[];
> >   } __packed;
> >   /* Disconnect Event */
> 
> whether or not you modify the length check consider this:
> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Great. :)

Thanks a lot for the feedback!
--
Gustavo

[PATCH 5/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_disconnect_event

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
wmi_disconnect_event.

It's also worth noting that due to the flexible array transformation,
the size of struct wmi_disconnect_event changed (now the size is 1 byte
smaller), and in order to preserve the logic of before the transformation,
the following change is needed:

        -       if (len < sizeof(struct wmi_disconnect_event))
        +       if (len <= sizeof(struct wmi_disconnect_event))

This issue was found with the help of Coccinelle and audited and fixed,
manually.

Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Hi!

It'd be great if someone can confirm or comment on the following
changes described in the changelog text:

        -       if (len < sizeof(struct wmi_disconnect_event))
        +       if (len <= sizeof(struct wmi_disconnect_event))

Thanks

 drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
 drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index ccdccead688e..645fb6cae3be 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1023,7 +1023,7 @@ static int ath6kl_wmi_disconnect_event_rx(struct wmi *wmi, u8 *datap, int len,
 	struct wmi_disconnect_event *ev;
 	wmi->traffic_class = 100;
 
-	if (len < sizeof(struct wmi_disconnect_event))
+	if (len <= sizeof(struct wmi_disconnect_event))
 		return -EINVAL;
 
 	ev = (struct wmi_disconnect_event *) datap;
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index 6b064e669d87..6a7fc07cd9aa 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -1596,7 +1596,7 @@ struct wmi_disconnect_event {
 	u8 disconn_reason;
 
 	u8 assoc_resp_len;
-	u8 assoc_info[1];
+	u8 assoc_info[];
 } __packed;
 
 /*
-- 
2.27.0

Re: [PATCH 5/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_disconnect_event

[Jeff Johnson]

On 2/22/2022 6:39 PM, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> wmi_disconnect_event.
> 
> It's also worth noting that due to the flexible array transformation,
> the size of struct wmi_disconnect_event changed (now the size is 1 byte
> smaller), and in order to preserve the logic of before the transformation,
> the following change is needed:
> 
>          -       if (len < sizeof(struct wmi_disconnect_event))
>          +       if (len <= sizeof(struct wmi_disconnect_event))
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Hi!
> 
> It'd be great if someone can confirm or comment on the following
> changes described in the changelog text:
> 
>          -       if (len < sizeof(struct wmi_disconnect_event))
>          +       if (len <= sizeof(struct wmi_disconnect_event))
> 
> Thanks
> 
>   drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
>   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index ccdccead688e..645fb6cae3be 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -1023,7 +1023,7 @@ static int ath6kl_wmi_disconnect_event_rx(struct wmi *wmi, u8 *datap, int len,
>   	struct wmi_disconnect_event *ev;
>   	wmi->traffic_class = 100;
>   
> -	if (len < sizeof(struct wmi_disconnect_event))
> +	if (len <= sizeof(struct wmi_disconnect_event))

this is another case where I believe the original code should remain 
since that is checking that the "fixed" portion is present.

and here again what is missing in the original code is logic to verify 
that the provide len is large enough to handle the advertised assoc_resp_len

>   		return -EINVAL;
>   
>   	ev = (struct wmi_disconnect_event *) datap;
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> index 6b064e669d87..6a7fc07cd9aa 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -1596,7 +1596,7 @@ struct wmi_disconnect_event {
>   	u8 disconn_reason;
>   
>   	u8 assoc_resp_len;
> -	u8 assoc_info[1];
> +	u8 assoc_info[];
>   } __packed;
>   
>   /*

[PATCH 6/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_aplist_event

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
wmi_aplist_event.

It's also worth noting that due to the flexible array transformation,
the size of struct wmi_aplist_event changed (now the size is 8-byte
smaller), and in order to preserve the logic of before the transformation,
the following change is needed:

        -       if (len < sizeof(struct wmi_aplist_event))
        +       if (len <= sizeof(struct wmi_aplist_event))

sizeof(struct wmi_aplist_event) before the flex-array transformation:

struct wmi_aplist_event {
	u8                         ap_list_ver;          /*     0     1 */
	u8                         num_ap;               /*     1     1 */
	union wmi_ap_info          ap_list[1];           /*     2     8 */

	/* size: 10, cachelines: 1, members: 3 */
	/* last cacheline: 10 bytes */
};

sizeof(struct wmi_aplist_event) after the flex-array transformation:

struct wmi_aplist_event {
	u8                         ap_list_ver;          /*     0     1 */
	u8                         num_ap;               /*     1     1 */
	union wmi_ap_info          ap_list[];            /*     2     0 */

	/* size: 2, cachelines: 1, members: 3 */
	/* last cacheline: 2 bytes */
};

Also, make use of the struct_size() helper and remove unneeded variable
ap_info_entry_size.

This issue was found with the help of Coccinelle and audited and fixed,
manually.

Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Hi!

It'd be great if someone can confirm or comment on the following
changes described in the changelog text:

        -       if (len < sizeof(struct wmi_aplist_event))
        +       if (len <= sizeof(struct wmi_aplist_event))

Thanks

 drivers/net/wireless/ath/ath6kl/wmi.c | 7 ++-----
 drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index 645fb6cae3be..484d37e66ce6 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1750,23 +1750,20 @@ static int ath6kl_wmi_snr_threshold_event_rx(struct wmi *wmi, u8 *datap,
 
 static int ath6kl_wmi_aplist_event_rx(struct wmi *wmi, u8 *datap, int len)
 {
-	u16 ap_info_entry_size;
 	struct wmi_aplist_event *ev = (struct wmi_aplist_event *) datap;
 	struct wmi_ap_info_v1 *ap_info_v1;
 	u8 index;
 
-	if (len < sizeof(struct wmi_aplist_event) ||
+	if (len <= sizeof(struct wmi_aplist_event) ||
 	    ev->ap_list_ver != APLIST_VER1)
 		return -EINVAL;
 
-	ap_info_entry_size = sizeof(struct wmi_ap_info_v1);
 	ap_info_v1 = (struct wmi_ap_info_v1 *) ev->ap_list;
 
 	ath6kl_dbg(ATH6KL_DBG_WMI,
 		   "number of APs in aplist event: %d\n", ev->num_ap);
 
-	if (len < (int) (sizeof(struct wmi_aplist_event) +
-			 (ev->num_ap - 1) * ap_info_entry_size))
+	if (len < struct_size(ev, ap_list, ev->num_ap))
 		return -EINVAL;
 
 	/* AP list version 1 contents */
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
index 6a7fc07cd9aa..a9732660192a 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.h
+++ b/drivers/net/wireless/ath/ath6kl/wmi.h
@@ -1957,7 +1957,7 @@ union wmi_ap_info {
 struct wmi_aplist_event {
 	u8 ap_list_ver;
 	u8 num_ap;
-	union wmi_ap_info ap_list[1];
+	union wmi_ap_info ap_list[];
 } __packed;
 
 /* Developer Commands */
-- 
2.27.0

Re: [PATCH 6/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_aplist_event

[Jeff Johnson]

On 2/22/2022 6:39 PM, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> wmi_aplist_event.
> 
> It's also worth noting that due to the flexible array transformation,
> the size of struct wmi_aplist_event changed (now the size is 8-byte
> smaller), and in order to preserve the logic of before the transformation,
> the following change is needed:
> 
>          -       if (len < sizeof(struct wmi_aplist_event))
>          +       if (len <= sizeof(struct wmi_aplist_event))
> 
> sizeof(struct wmi_aplist_event) before the flex-array transformation:
> 
> struct wmi_aplist_event {
> 	u8                         ap_list_ver;          /*     0     1 */
> 	u8                         num_ap;               /*     1     1 */
> 	union wmi_ap_info          ap_list[1];           /*     2     8 */
> 
> 	/* size: 10, cachelines: 1, members: 3 */
> 	/* last cacheline: 10 bytes */
> };
> 
> sizeof(struct wmi_aplist_event) after the flex-array transformation:
> 
> struct wmi_aplist_event {
> 	u8                         ap_list_ver;          /*     0     1 */
> 	u8                         num_ap;               /*     1     1 */
> 	union wmi_ap_info          ap_list[];            /*     2     0 */
> 
> 	/* size: 2, cachelines: 1, members: 3 */
> 	/* last cacheline: 2 bytes */
> };
> 
> Also, make use of the struct_size() helper and remove unneeded variable
> ap_info_entry_size.
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Hi!
> 
> It'd be great if someone can confirm or comment on the following
> changes described in the changelog text:
> 
>          -       if (len < sizeof(struct wmi_aplist_event))
>          +       if (len <= sizeof(struct wmi_aplist_event))
> 
> Thanks
> 
>   drivers/net/wireless/ath/ath6kl/wmi.c | 7 ++-----
>   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
>   2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index 645fb6cae3be..484d37e66ce6 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -1750,23 +1750,20 @@ static int ath6kl_wmi_snr_threshold_event_rx(struct wmi *wmi, u8 *datap,
>   
>   static int ath6kl_wmi_aplist_event_rx(struct wmi *wmi, u8 *datap, int len)
>   {
> -	u16 ap_info_entry_size;
>   	struct wmi_aplist_event *ev = (struct wmi_aplist_event *) datap;
>   	struct wmi_ap_info_v1 *ap_info_v1;
>   	u8 index;
>   
> -	if (len < sizeof(struct wmi_aplist_event) ||
> +	if (len <= sizeof(struct wmi_aplist_event) ||

again IMO the original code is preferred since then we can handle a 
0-length list

>   	    ev->ap_list_ver != APLIST_VER1)
>   		return -EINVAL;
>   
> -	ap_info_entry_size = sizeof(struct wmi_ap_info_v1);
>   	ap_info_v1 = (struct wmi_ap_info_v1 *) ev->ap_list;
>   
>   	ath6kl_dbg(ATH6KL_DBG_WMI,
>   		   "number of APs in aplist event: %d\n", ev->num_ap);
>   
> -	if (len < (int) (sizeof(struct wmi_aplist_event) +
> -			 (ev->num_ap - 1) * ap_info_entry_size))
> +	if (len < struct_size(ev, ap_list, ev->num_ap))

and unlike the prior patches in this set, at least the original code 
here had logic to validate len against the metadata that describes the 
number of entries in the list. so this change is good, and also supports 
a 0-length list

>   		return -EINVAL;
>   
>   	/* AP list version 1 contents */
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> index 6a7fc07cd9aa..a9732660192a 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -1957,7 +1957,7 @@ union wmi_ap_info {
>   struct wmi_aplist_event {
>   	u8 ap_list_ver;
>   	u8 num_ap;
> -	union wmi_ap_info ap_list[1];
> +	union wmi_ap_info ap_list[];
>   } __packed;
>   
>   /* Developer Commands */

whether or not you modify the length check consider this:
Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>