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 <[email protected]>
---
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
"Gustavo A. R. Silva" <[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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)
>> - 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