| Message ID | ZgRayuCJ0gQinMvr@neat (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2,next] wifi: wil6210: Annotate a couple of structs with __counted_by() | expand |
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 */
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 */ > >
>>> #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
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 */
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 */
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(-)