[PATCH] wifi: wireless: fix more UBSAN noise in cfg80211_conn_scan()

[PATCH] wifi: wireless: fix more UBSAN noise in cfg80211_conn_scan()

[Dmitry Antipov]

Looking at https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774
and trying to reproduce it with CONFIG_UBSAN enabled, I've noticed the
following:

UBSAN: array-index-out-of-bounds in net/wireless/sme.c:95:3
index 0 is out of range for type 'struct ieee80211_channel *[]'
CPU: 3 PID: 4993 Comm: repro Not tainted 6.10.0-01155-gd67978318827 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <...>
Call Trace:
 <TASK>
 dump_stack_lvl+0x1c2/0x2a0
 ? __pfx_dump_stack_lvl+0x10/0x10
 ? __pfx__printk+0x10/0x10
 ? __local_bh_enable_ip+0x12e/0x1c0
 __ubsan_handle_out_of_bounds+0x127/0x150
 cfg80211_conn_scan+0xd8e/0xf30
 cfg80211_connect+0x1400/0x1c30
 nl80211_connect+0x1549/0x1a70
 ...<the rest is not too useful...>

This is very similar to 92ecbb3ac6f3 ("wifi: mac80211: fix UBSAN noise
in ieee80211_prep_hw_scan()"), so just fix it in the same way by setting
'request->n_channels' early to help '__counted_by()' work as expected.
And the same 'kmalloc()' math adjustment is also applicable.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 net/wireless/sme.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

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);
 	if (!request)
 		return -ENOMEM;
 
+	/* None of the channels are actually set
+	 * up but let UBSAN know the boundaries.
+	 */
+	request->n_channels = n_channels;
+
 	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;
 	}
-	request->n_channels = n_channels;
+
 	request->ssids = (void *)&request->channels[n_channels];
 	request->n_ssids = 1;
 
-- 
2.45.2

Re: [PATCH] wifi: wireless: fix more UBSAN noise in cfg80211_conn_scan()

[Kees Cook]

On Tue, Jul 16, 2024 at 08:40:11PM +0300, Dmitry Antipov wrote:
> Looking at https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774
> and trying to reproduce it with CONFIG_UBSAN enabled, I've noticed the
> following:
> 
> UBSAN: array-index-out-of-bounds in net/wireless/sme.c:95:3
> index 0 is out of range for type 'struct ieee80211_channel *[]'
> CPU: 3 PID: 4993 Comm: repro Not tainted 6.10.0-01155-gd67978318827 #5
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <...>
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x1c2/0x2a0
>  ? __pfx_dump_stack_lvl+0x10/0x10
>  ? __pfx__printk+0x10/0x10
>  ? __local_bh_enable_ip+0x12e/0x1c0
>  __ubsan_handle_out_of_bounds+0x127/0x150
>  cfg80211_conn_scan+0xd8e/0xf30
>  cfg80211_connect+0x1400/0x1c30
>  nl80211_connect+0x1549/0x1a70
>  ...<the rest is not too useful...>
> 
> This is very similar to 92ecbb3ac6f3 ("wifi: mac80211: fix UBSAN noise
> in ieee80211_prep_hw_scan()"), so just fix it in the same way by setting
> 'request->n_channels' early to help '__counted_by()' work as expected.
> And the same 'kmalloc()' math adjustment is also applicable.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Nice catch! Yes, this looks correct.

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: [PATCH] wifi: wireless: fix more UBSAN noise in cfg80211_conn_scan()

[Johannes Berg]

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