[PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by()

[PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by()

[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).

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
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

Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by()

[Jeff Johnson]

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 <gustavoars@kernel.org>
> ---
> 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:

		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 */

Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by()

[Gustavo A. R. Silva]

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 <gustavoars@kernel.org>
>> ---
>> 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 */
> 
> 

Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by()

[Gustavo A. R. Silva]

>>>   #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])
>>

I will merge these two patches together:

https://lore.kernel.org/linux-hardening/ZgODZOB4fOBvKl7R@neat/
https://lore.kernel.org/linux-hardening/ZgOEoCWguq3n1OqQ@neat/

and send these changes together with the DEFINE_FLEX() transformation
in drivers/net/wireless/ath/wil6210/cfg80211.c

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;

Thanks
--
Gustavo

Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by()

[Gustavo A. R. Silva]

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 <gustavoars@kernel.org>
> ---
> 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 */