[PATCH][next] wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning

[PATCH][next] wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning

[Gustavo A. R. Silva]

Prepare for the coming implementation by GCC and Clang of the
__counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time
via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
(for strcpy/memcpy-family functions).

Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
getting ready to enable it globally.

So, use the `DEFINE_FLEX()` helper for an on-stack definition of
a flexible structure where the size of the flexible-array member
is known at compile-time, and refactor the rest of the code,
accordingly.

So, with these changes, fix the following warning:
drivers/net/wireless/ath/wil6210/cfg80211.c:896:43: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++++++++-----------
 drivers/net/wireless/ath/wil6210/wmi.h      |  2 +-
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index dbe4b3478f03..836b49954171 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -892,10 +892,8 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
 	struct wil6210_priv *wil = wiphy_to_wil(wiphy);
 	struct wireless_dev *wdev = request->wdev;
 	struct wil6210_vif *vif = wdev_to_vif(wil, wdev);
-	struct {
-		struct wmi_start_scan_cmd cmd;
-		u16 chnl[4];
-	} __packed cmd;
+	DEFINE_FLEX(struct wmi_start_scan_cmd, cmd,
+		    channel_list, num_channels, 4);
 	uint i, n;
 	int rc;
 
@@ -977,9 +975,9 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
 	vif->scan_request = request;
 	mod_timer(&vif->scan_timer, jiffies + WIL6210_SCAN_TO);
 
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.cmd.scan_type = WMI_ACTIVE_SCAN;
-	cmd.cmd.num_channels = 0;
+	memset(cmd, 0, sizeof(*cmd));
+	cmd->scan_type = WMI_ACTIVE_SCAN;
+	cmd->num_channels = 0;
 	n = min(request->n_channels, 4U);
 	for (i = 0; i < n; i++) {
 		int ch = request->channels[i]->hw_value;
@@ -991,7 +989,8 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
 			continue;
 		}
 		/* 0-based channel indexes */
-		cmd.cmd.channel_list[cmd.cmd.num_channels++].channel = ch - 1;
+		cmd->num_channels++;
+		cmd->channel_list[cmd->num_channels - 1].channel = ch - 1;
 		wil_dbg_misc(wil, "Scan for ch %d  : %d MHz\n", ch,
 			     request->channels[i]->center_freq);
 	}
@@ -1007,16 +1006,15 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
 	if (rc)
 		goto out_restore;
 
-	if (wil->discovery_mode && cmd.cmd.scan_type == WMI_ACTIVE_SCAN) {
-		cmd.cmd.discovery_mode = 1;
+	if (wil->discovery_mode && cmd->scan_type == WMI_ACTIVE_SCAN) {
+		cmd->discovery_mode = 1;
 		wil_dbg_misc(wil, "active scan with discovery_mode=1\n");
 	}
 
 	if (vif->mid == 0)
 		wil->radio_wdev = wdev;
 	rc = wmi_send(wil, WMI_START_SCAN_CMDID, vif->mid,
-		      &cmd, sizeof(cmd.cmd) +
-		      cmd.cmd.num_channels * sizeof(cmd.cmd.channel_list[0]));
+		      cmd, struct_size(cmd, channel_list, cmd->num_channels));
 
 out_restore:
 	if (rc) {
diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
index 71bf2ae27a98..b47606d9068c 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.h
+++ b/drivers/net/wireless/ath/wil6210/wmi.h
@@ -474,7 +474,7 @@ struct wmi_start_scan_cmd {
 	struct {
 		u8 channel;
 		u8 reserved;
-	} channel_list[];
+	} channel_list[] __counted_by(num_channels);
 } __packed;
 
 #define WMI_MAX_PNO_SSID_NUM	(16)
-- 
2.34.1

Re: [PATCH][next] wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning

[Kalle Valo]

"Gustavo A. R. Silva" <gustavoars@kernel.org> writes:

> Prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time
> via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
>
> Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> getting ready to enable it globally.
>
> So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> a flexible structure where the size of the flexible-array member
> is known at compile-time, and refactor the rest of the code,
> accordingly.
>
> So, with these changes, fix the following warning:
> drivers/net/wireless/ath/wil6210/cfg80211.c:896:43: warning: structure
> containing a flexible array member is not at the end of another
> structure [-Wflex-array-member-not-at-end]
>
> Link: https://github.com/KSPP/linux/issues/202
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++++++++-----------
>  drivers/net/wireless/ath/wil6210/wmi.h      |  2 +-
>  2 files changed, 11 insertions(+), 13 deletions(-)

The subject should being with "wifi: wil6210:", I can fix that. (Didn't
review the rest yet.)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH][next] wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning

[Gustavo A. R. Silva]

> The subject should being with "wifi: wil6210:", I can fix that. (Didn't
> review the rest yet.)
> 

It seems I got it right in a subsequent patch. :)

Thanks, Kalle.
--
Gustavo

Re: [PATCH][next] wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning

[Jeff Johnson]

On 3/27/2024 11:50 AM, Gustavo A. R. Silva wrote:
> Prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time
> via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
> 
> Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> getting ready to enable it globally.
> 
> So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> a flexible structure where the size of the flexible-array member
> is known at compile-time, and refactor the rest of the code,
> accordingly.
> 
> So, with these changes, fix the following warning:
> drivers/net/wireless/ath/wil6210/cfg80211.c:896:43: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Link: https://github.com/KSPP/linux/issues/202
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++++++++-----------
>  drivers/net/wireless/ath/wil6210/wmi.h      |  2 +-
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
> index dbe4b3478f03..836b49954171 100644
> --- a/drivers/net/wireless/ath/wil6210/cfg80211.c
> +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
> @@ -892,10 +892,8 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
>  	struct wil6210_priv *wil = wiphy_to_wil(wiphy);
>  	struct wireless_dev *wdev = request->wdev;
>  	struct wil6210_vif *vif = wdev_to_vif(wil, wdev);
> -	struct {
> -		struct wmi_start_scan_cmd cmd;
> -		u16 chnl[4];
> -	} __packed cmd;
> +	DEFINE_FLEX(struct wmi_start_scan_cmd, cmd,
> +		    channel_list, num_channels, 4);
>  	uint i, n;
>  	int rc;
>  
> @@ -977,9 +975,9 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
>  	vif->scan_request = request;
>  	mod_timer(&vif->scan_timer, jiffies + WIL6210_SCAN_TO);
>  
> -	memset(&cmd, 0, sizeof(cmd));
> -	cmd.cmd.scan_type = WMI_ACTIVE_SCAN;
> -	cmd.cmd.num_channels = 0;
> +	memset(cmd, 0, sizeof(*cmd));

Isn't this unnecessary since DEFINE_FLEX() logic "{ .obj.COUNTER = COUNT, }"
should result in everything else being zeroed?

And if that isn't sufficient, DEFINE_FLEX() itself says we should "Use
__struct_size(@NAME) to get compile-time size of it afterwards"

Note the current memset won't zero the flex array and hence if the actual
number of channels is less than 4 then kernel stack contents could be exposed
to firmware.

> +	cmd->scan_type = WMI_ACTIVE_SCAN;
> +	cmd->num_channels = 0;
>  	n = min(request->n_channels, 4U);
>  	for (i = 0; i < n; i++) {
>  		int ch = request->channels[i]->hw_value;
> @@ -991,7 +989,8 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
>  			continue;
>  		}
>  		/* 0-based channel indexes */
> -		cmd.cmd.channel_list[cmd.cmd.num_channels++].channel = ch - 1;
> +		cmd->num_channels++;
> +		cmd->channel_list[cmd->num_channels - 1].channel = ch - 1;
>  		wil_dbg_misc(wil, "Scan for ch %d  : %d MHz\n", ch,
>  			     request->channels[i]->center_freq);
>  	}
> @@ -1007,16 +1006,15 @@ static int wil_cfg80211_scan(struct wiphy *wiphy,
>  	if (rc)
>  		goto out_restore;
>  
> -	if (wil->discovery_mode && cmd.cmd.scan_type == WMI_ACTIVE_SCAN) {
> -		cmd.cmd.discovery_mode = 1;
> +	if (wil->discovery_mode && cmd->scan_type == WMI_ACTIVE_SCAN) {
> +		cmd->discovery_mode = 1;
>  		wil_dbg_misc(wil, "active scan with discovery_mode=1\n");
>  	}
>  
>  	if (vif->mid == 0)
>  		wil->radio_wdev = wdev;
>  	rc = wmi_send(wil, WMI_START_SCAN_CMDID, vif->mid,
> -		      &cmd, sizeof(cmd.cmd) +
> -		      cmd.cmd.num_channels * sizeof(cmd.cmd.channel_list[0]));
> +		      cmd, struct_size(cmd, channel_list, cmd->num_channels));
>  
>  out_restore:
>  	if (rc) {
> diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
> index 71bf2ae27a98..b47606d9068c 100644
> --- a/drivers/net/wireless/ath/wil6210/wmi.h
> +++ b/drivers/net/wireless/ath/wil6210/wmi.h
> @@ -474,7 +474,7 @@ struct wmi_start_scan_cmd {
>  	struct {
>  		u8 channel;
>  		u8 reserved;
> -	} channel_list[];
> +	} channel_list[] __counted_by(num_channels);
>  } __packed;
>  
>  #define WMI_MAX_PNO_SSID_NUM	(16)

Re: [PATCH][next] wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning

[Gustavo A. R. Silva]

>> -	memset(&cmd, 0, sizeof(cmd));
>> -	cmd.cmd.scan_type = WMI_ACTIVE_SCAN;
>> -	cmd.cmd.num_channels = 0;
>> +	memset(cmd, 0, sizeof(*cmd));
> 
> Isn't this unnecessary since DEFINE_FLEX() logic "{ .obj.COUNTER = COUNT, }"
> should result in everything else being zeroed?
> 
> And if that isn't sufficient, DEFINE_FLEX() itself says we should "Use
> __struct_size(@NAME) to get compile-time size of it afterwards"
> 
> Note the current memset won't zero the flex array and hence if the actual
> number of channels is less than 4 then kernel stack contents could be exposed
> to firmware.

Yes, that's correct. The current memset() will only zero out a total of
sizeof(struct wmi_start_scan_cmd) bytes, which of course doesn't include
the flex-array member.

Thanks for the review, I'll remove that line!

--
Gustavo