* [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() @ 2024-03-27 17:43 Gustavo A. R. Silva 2024-03-27 18:26 ` Jeff Johnson 2024-03-27 18:53 ` Gustavo A. R. Silva 0 siblings, 2 replies; 5+ messages in thread From: Gustavo A. R. Silva @ 2024-03-27 17:43 UTC (permalink / raw) To: Kalle Valo Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva, linux-hardening 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() 2024-03-27 17:43 [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() Gustavo A. R. Silva @ 2024-03-27 18:26 ` Jeff Johnson 2024-03-27 18:34 ` Gustavo A. R. Silva 2024-03-27 18:53 ` Gustavo A. R. Silva 1 sibling, 1 reply; 5+ messages in thread From: Jeff Johnson @ 2024-03-27 18:26 UTC (permalink / raw) To: Gustavo A. R. Silva, Kalle Valo Cc: linux-wireless, linux-kernel, linux-hardening 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 */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() 2024-03-27 18:26 ` Jeff Johnson @ 2024-03-27 18:34 ` Gustavo A. R. Silva 2024-03-27 18:39 ` Gustavo A. R. Silva 0 siblings, 1 reply; 5+ messages in thread From: Gustavo A. R. Silva @ 2024-03-27 18:34 UTC (permalink / raw) To: Jeff Johnson, Gustavo A. R. Silva, Kalle Valo Cc: linux-wireless, linux-kernel, linux-hardening 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 */ > > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() 2024-03-27 18:34 ` Gustavo A. R. Silva @ 2024-03-27 18:39 ` Gustavo A. R. Silva 0 siblings, 0 replies; 5+ messages in thread From: Gustavo A. R. Silva @ 2024-03-27 18:39 UTC (permalink / raw) To: Jeff Johnson, Gustavo A. R. Silva, Kalle Valo Cc: linux-wireless, linux-kernel, linux-hardening >>> #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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() 2024-03-27 17:43 [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() Gustavo A. R. Silva 2024-03-27 18:26 ` Jeff Johnson @ 2024-03-27 18:53 ` Gustavo A. R. Silva 1 sibling, 0 replies; 5+ messages in thread From: Gustavo A. R. Silva @ 2024-03-27 18:53 UTC (permalink / raw) To: Gustavo A. R. Silva, Kalle Valo Cc: linux-wireless, linux-kernel, linux-hardening 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 */ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-27 18:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-27 17:43 [PATCH v2][next] wifi: wil6210: Annotate a couple of structs with __counted_by() Gustavo A. R. Silva 2024-03-27 18:26 ` Jeff Johnson 2024-03-27 18:34 ` Gustavo A. R. Silva 2024-03-27 18:39 ` Gustavo A. R. Silva 2024-03-27 18:53 ` Gustavo A. R. Silva
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.