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).
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Annotate one more struct.
- Update Subject line.
v1:
- Link: https://lore.kernel.org/linux-hardening/ZgODZOB4fOBvKl7R@neat/
drivers/net/wireless/ath/wil6210/wmi.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
index 71bf2ae27a98..38f64524019e 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)
@@ -3320,7 +3320,7 @@ struct wmi_set_link_monitor_cmd {
u8 rssi_hyst;
u8 reserved[12];
u8 rssi_thresholds_list_size;
- s8 rssi_thresholds_list[];
+ s8 rssi_thresholds_list[] __counted_by(rssi_thresholds_list_size);
} __packed;
/* wmi_link_monitor_event_type */
--
2.34.1
On 3/27/24 12:26, Jeff Johnson wrote:
> On 3/27/2024 10:43 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).
>>
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>> ---
>> Changes in v2:
>> - Annotate one more struct.
>> - Update Subject line.
>>
>> v1:
>> - Link: https://lore.kernel.org/linux-hardening/ZgODZOB4fOBvKl7R@neat/
>>
>> drivers/net/wireless/ath/wil6210/wmi.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
>> index 71bf2ae27a98..38f64524019e 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;
>
> does the compiler handle the actual logic where it is modifying num_channels
> concurrently with writing into the array? i.e. this will be writing into
> channel_list[0] when num_channels is 0:
I'm actually about to send this patch:
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) {
--
Gustavo
>
> cmd.cmd.channel_list[cmd.cmd.num_channels++].channel = ch - 1;
>
> if that will cause a bounds check failure then suggest you change the logic so
> that it updates num_channels before writing into channel_list
>
>>
>> #define WMI_MAX_PNO_SSID_NUM (16)
>> @@ -3320,7 +3320,7 @@ struct wmi_set_link_monitor_cmd {
>> u8 rssi_hyst;
>> u8 reserved[12];
>> u8 rssi_thresholds_list_size;
>> - s8 rssi_thresholds_list[];
>> + s8 rssi_thresholds_list[] __counted_by(rssi_thresholds_list_size);
>> } __packed;
>
> this looks ok to me, although I think there is another issue associated with
> this, namely the way the code populates the rssi_thresholds_list is by
> defining a separate anonymous struct:
> struct {
> struct wmi_set_link_monitor_cmd cmd;
> s8 rssi_thold;
> } __packed cmd = {
> .cmd = {
> .rssi_hyst = rssi_hyst,
> .rssi_thresholds_list_size = 1,
> },
> .rssi_thold = rssi_thold,
> };
>
> I would expect gcc and clang to both complain about that s8 rssi_thold comes
> after a flexible array (even though its purpose is to be the value of
> rssi_thresholds_list[0])
>
> /jeff
>
>
>>
>> /* wmi_link_monitor_event_type */
>
>
Hi all,
Please, drop this.
The following patches replaces it:
https://lore.kernel.org/linux-hardening/ZgRqjGShTl3y5FFB@neat/
Thanks
--
Gustavo
On 3/27/24 11:43, 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).
>
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> Changes in v2:
> - Annotate one more struct.
> - Update Subject line.
>
> v1:
> - Link: https://lore.kernel.org/linux-hardening/ZgODZOB4fOBvKl7R@neat/
>
> drivers/net/wireless/ath/wil6210/wmi.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
> index 71bf2ae27a98..38f64524019e 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)
> @@ -3320,7 +3320,7 @@ struct wmi_set_link_monitor_cmd {
> u8 rssi_hyst;
> u8 reserved[12];
> u8 rssi_thresholds_list_size;
> - s8 rssi_thresholds_list[];
> + s8 rssi_thresholds_list[] __counted_by(rssi_thresholds_list_size);
> } __packed;
>
> /* wmi_link_monitor_event_type */