netdev - Re: [net/wireless] Question about `cfg80211_conn_scan` func: misuse of __counted

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <99567c3c-1f45-4a3f-a739-b35f014127b5@linux.dev>
Date: Tue, 20 Aug 2024 09:59:19 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Haoyu Li <lihaoyu499@...il.com>
Cc: netdev@...r.kernel.org, Johannes Berg <johannes@...solutions.net>,
 "David S. Miller" <davem@...emloft.net>, linux-wireless@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [net/wireless] Question about `cfg80211_conn_scan` func: misuse
 of __counted_by

On 19/08/2024 20:19, Haoyu Li wrote:
> Dear Linux Developers for NETWORKING and CFG80211/NL80211,
> 
> We are curious about the use of `struct cfg80211_scan_request *request`
> in function `cfg80211_conn_scan`.
> The definition of `struct cfg80211_scan_request` is at
> https://elixir.bootlin.com/linux/v6.10.6/source/include/net/cfg80211.h#L2675.
> ```
> struct cfg80211_scan_request {
> struct cfg80211_ssid *ssids;
> int n_ssids;
> u32 n_channels;
> const u8 *ie;
> size_t ie_len;
> u16 duration;
> bool duration_mandatory;
> u32 flags;
> 
> u32 rates[NUM_NL80211_BANDS];
> 
> struct wireless_dev *wdev;
> 
> u8 mac_addr[ETH_ALEN] __aligned(2);
> u8 mac_addr_mask[ETH_ALEN] __aligned(2);
> u8 bssid[ETH_ALEN] __aligned(2);
> 
> /* internal */
> struct wiphy *wiphy;
> unsigned long scan_start;
> struct cfg80211_scan_info info;
> bool notified;
> bool no_cck;
> bool scan_6ghz;
> u32 n_6ghz_params;
> struct cfg80211_scan_6ghz_params *scan_6ghz_params;
> s8 tsf_report_link_id;
> 
> /* keep last */
> struct ieee80211_channel *channels[] __counted_by(n_channels);
> };
> ```
> 
> Our question is: The `channels` member of `struct
> cfg80211_scan_request` is annotated
> with "__counted_by", which means the array size is indicated by
> `n_channels`. Only if we set `n_channels` before accessing
> `channels[i]`, the flexible
> member `hws` can be properly bounds-checked at run-time when enabling
> CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. Or there will be a
> warning from each array access that is prior to the initialization
> because the number of elements is zero.
> 
> In function `cfg80211_conn_scan` at
> https://elixir.bootlin.com/linux/v6.10.6/source/net/wireless/sme.c#L117,
> we think it's needed to relocate `request->n_channels = n_channels` before
> accessing `request->channels[...]`.
> 
> Here is a fix example of a similar situation :
> https://lore.kernel.org/stable/20240613113225.898955993@linuxfoundation.org/.
> 
> Please kindly correct us if we missed any key information. Looking
> forward to your response!

You are quite right that the case when (wdev->conn->params.channel !=
NULL) should initialize n_channels to 1 first.

The other question is if it's legal to take address beyond the end of
array. I'm talking about
request->ssids = (void *)&request->channels[n_channels];
But you can easily check it yourself with pretty simple program.



> Best,
> Haoyu Li