Skip to contentSkip to conversation

wifi: wil6210: Annotate a couple of structs with __counted_by()

PATCH In review Gustavo A. R. Silva 3 participants 5 messages Mar 27, 2024 Wilocity WIL6210 wireless driver

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH v2 1/1 wifi: wil6210: Annotate a couple of structs with __counted_by()

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(-)
drivers/net/wireless/ath/wil6210/wmi.h +2 -2
index 71bf2ae27a98..38f64524019e 100644
@@ -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 */
Signature · 1 line
2.34.1
#1 lore raw
Quoted 23 lines · 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 <[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(-)
drivers/net/wireless/ath/wil6210/wmi.h +0 -0
index 71bf2ae27a98..38f64524019e 100644

drivers/net/wireless/ath/wil6210/wmi.h struct wmi_start_scan_cmd { · line 478

Quoted 6 lines
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;

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

drivers/net/wireless/ath/wil6210/wmi.h struct wmi_set_link_monitor_cmd { · line 3324

Quoted 9 lines
drivers/net/wireless/ath/wil6210/wmi.h +1 -1
 #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

Quoted 2 lines
drivers/net/wireless/ath/wil6210/wmi.h +0 -0
 /* wmi_link_monitor_event_type */
#2 lore raw
Quoted 19 lines · Jeff Johnson
Quoted 18 lines · 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 <[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(-)
Quoted 15 lines

drivers/net/wireless/ath/wil6210/wmi.h struct wmi_start_scan_cmd { · line 478

Quoted 11 lines
drivers/net/wireless/ath/wil6210/wmi.h +1 -1
index 71bf2ae27a98..38f64524019e 100644
@@ -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:

drivers/net/wireless/ath/wil6210/cfg80211.c +10 -12
index dbe4b3478f03..836b49954171 100644
@@ -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

Quoted 41 lines
		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

drivers/net/wireless/ath/wil6210/wmi.h struct wmi_set_link_monitor_cmd { · line 3324

Quoted 9 lines
drivers/net/wireless/ath/wil6210/wmi.h +1 -1
  #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

Quoted 2 lines
drivers/net/wireless/ath/wil6210/wmi.h +0 -0
  /* wmi_link_monitor_event_type */
#3 lore raw
Quoted 24 lines
Quoted 24 lines

Review struct wmi_set_link_monitor_cmd { · line 3324

Quoted 9 lines

  #define WMI_MAX_PNO_SSID_NUM    (16)

(quoted diff) +1 -1
@@ -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,
    };

Quoted 4 lines
Quoted 4 lines

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

drivers/net/wireless/ath/wil6210/wmi.h +1 -1
index 71bf2ae27a98..38f64524019e 100644
@@ -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

#4 lore raw
In reply to Gustavo A. R. Silva

Hi all,

Please, drop this.

The following patches replaces it:

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

Thanks
--
Gustavo

Quoted 40 lines · 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 <[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(-)
drivers/net/wireless/ath/wil6210/wmi.h +2 -2
index 71bf2ae27a98..38f64524019e 100644
@@ -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 */