wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning
PATCH 1/1 wifi: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid -Wfamnae warning
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]
--- drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++++++++----------- drivers/net/wireless/ath/wil6210/wmi.h | 2 +- 2 files changed, 11 insertions(+), 13 deletions(-)
drivers/net/wireless/ath/wil6210/cfg80211.c +10 -12
@@ -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) {
drivers/net/wireless/ath/wil6210/wmi.h +1 -1
@@ -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)
Signature · 1 line
2.34.1
Quoted 19 lines · Gustavo A. R. Silva
Prepare for the coming implementation by GCC and Clang of the
__counted_byattribute. Flexible array members annotated with__counted_bycan have their accesses bounds-checked at run-time viaCONFIG_UBSAN_BOUNDS(for array indexing) andCONFIG_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]
Quoted 6 lines
--- 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.)
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Quoted 4 lines
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
Quoted 42 lines · Gustavo A. R. Silva
Prepare for the coming implementation by GCC and Clang of the
__counted_byattribute. Flexible array members annotated with__counted_bycan have their accesses bounds-checked at run-time viaCONFIG_UBSAN_BOUNDS(for array indexing) andCONFIG_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]--- drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++++++++----------- drivers/net/wireless/ath/wil6210/wmi.h | 2 +- 2 files changed, 11 insertions(+), 13 deletions(-)drivers/net/wireless/ath/wil6210/cfg80211.c +2
-4@@ -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;
drivers/net/wireless/ath/wil6210/cfg80211.c static int wil_cfg80211_scan(struct wiphy *wiphy, · line 978
Quoted 7 lines
drivers/net/wireless/ath/wil6210/cfg80211.c +1
-3@@ -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.
Quoted 48 lines
drivers/net/wireless/ath/wil6210/cfg80211.c +7
-5+ 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) {drivers/net/wireless/ath/wil6210/wmi.h +1
-1@@ -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)
Quoted 9 lines
drivers/net/wireless/ath/wil6210/cfg80211.c static int wil_cfg80211_scan(struct wiphy *wiphy, · line 978
Quoted 5 lines
drivers/net/wireless/ath/wil6210/cfg80211.c +1
-3-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?
Quoted 6 lines
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