ath6kl: wmi: Replace one-element arrays with flexible-array members

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 <[email protected]>

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

drivers/net/wireless/ath/ath6kl/wmi.c +2 -7

index bd1ef6334997..e1c950014f3e 100644
@@ -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;
 

drivers/net/wireless/ath/ath6kl/wmi.h +0 -0

index 784940ba4c90..322539ed9c12 100644

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 <[email protected]>

---
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(-)

drivers/net/wireless/ath/ath6kl/wmi.c +1 -1

index bdfc057c5a82..049d75f31f3c 100644
@@ -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);

drivers/net/wireless/ath/ath6kl/wmi.h +0 -0

index 9e168752bec2..432e4f428a4a 100644

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 <[email protected]>

---
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(-)

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

index 049d75f31f3c..ccdccead688e 100644

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

  		return -EINVAL;
 
  	ev = (struct wmi_connect_event *) datap;

drivers/net/wireless/ath/ath6kl/wmi.h +0 -0

index 432e4f428a4a..6b064e669d87 100644

Quoted 32 lines · 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 <[email protected]>

---
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(-)

drivers/net/wireless/ath/ath6kl/wmi.c static int ath6kl_wmi_connect_event_rx(struct wmi *wmi, u8 *datap, int len, · line 859

Quoted 9 lines

drivers/net/wireless/ath/ath6kl/wmi.c +1 -1

index 049d75f31f3c..ccdccead688e 100644
@@ -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.

Quoted 7 lines

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

  		return -EINVAL;
  	ev = (struct wmi_connect_event *) datap;

drivers/net/wireless/ath/ath6kl/wmi.h +0 -0

index 432e4f428a4a..6b064e669d87 100644

drivers/net/wireless/ath/ath6kl/wmi.h struct wmi_connect_event { · line 1550

Quoted 7 lines

drivers/net/wireless/ath/ath6kl/wmi.h +1 -1

@@ -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 <[email protected]>

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 <[email protected]>

---
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(-)

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

index ccdccead688e..645fb6cae3be 100644

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

  		return -EINVAL;
 
  	ev = (struct wmi_disconnect_event *) datap;

drivers/net/wireless/ath/ath6kl/wmi.h +1 -1

index 6b064e669d87..6a7fc07cd9aa 100644
@@ -1596,7 +1596,7 @@ struct wmi_disconnect_event {
  	u8 disconn_reason;
 
  	u8 assoc_resp_len;
-	u8 assoc_info[1];
+	u8 assoc_info[];
  } __packed;
 
  /*

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 <[email protected]>

---
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(-)

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

index 645fb6cae3be..484d37e66ce6 100644

drivers/net/wireless/ath/ath6kl/wmi.c +0 -0

  		return -EINVAL;
 
  	/* AP list version 1 contents */

drivers/net/wireless/ath/ath6kl/wmi.h +0 -0

index 6a7fc07cd9aa..a9732660192a 100644