linux-hardening - Re: [PATCH] wifi: wireless: fix more UBSAN noise in cfg80211_conn

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: <3ddcf25ba09bf99c973c46ef1944c079be2753eb.camel@sipsolutions.net>
Date: Tue, 27 Aug 2024 10:12:26 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Dmitry Antipov <dmantipov@...dex.ru>
Cc: Kees Cook <kees@...nel.org>, linux-wireless@...r.kernel.org, 
	linux-hardening@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH] wifi: wireless: fix more UBSAN noise in
 cfg80211_conn_scan()

Hmm.

On Tue, 2024-07-16 at 20:40 +0300, Dmitry Antipov wrote:
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index a8ad55f11133..f5da45331847 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -77,12 +77,16 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev)
>  	else
>  		n_channels = ieee80211_get_num_supported_channels(wdev->wiphy);
>  
> -	request = kzalloc(sizeof(*request) + sizeof(request->ssids[0]) +
> -			  sizeof(request->channels[0]) * n_channels,
> -			  GFP_KERNEL);
> +	request = kzalloc(struct_size(request, channels, n_channels) +
> +			  sizeof(request->ssids[0]), GFP_KERNEL);

That makes sense, sure.

>  	if (!request)
>  		return -ENOMEM;
>  
> +	/* None of the channels are actually set
> +	 * up but let UBSAN know the boundaries.
> +	 */
> +	request->n_channels = n_channels;

Also makes sense, so we tell it how many we allocated early.

Note netdev we dropped the special comment style requirement, so
wouldn't mind

 /*
  * None of ...
  * ...
  */

here either.

> +
>  	if (wdev->conn->params.channel) {
>  		enum nl80211_band band = wdev->conn->params.channel->band;
>  		struct ieee80211_supported_band *sband =
> @@ -112,9 +116,9 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev)
>  			}
>  			request->rates[band] = (1 << bands->n_bitrates) - 1;
>  		}
> -		n_channels = i;
> +		request->n_channels = i;

So this tells it how many were actually used, in this branch, makes
sense.

Functionally, all of this seems OK.

However,

>  	request->ssids = (void *)&request->channels[n_channels];

is this not checked? I mean, if you have n_channels=5 and then take
&channels[5] I can see how that makes sense, but arguably the compiler
might complain if you have &channels[10] for an array you told it has 5
entries?

johannes