Messages in this thread Patch in this message | | | Date | Wed, 27 Mar 2024 12:34:14 -0600 | Subject | Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() | From | "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 */ > >
| |