[PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Kees Cook]

Before request->channels[] can be used, request->n_channels must be set.
Additionally, address calculations for memory after the "channels" array
need to be calculated from the allocation base ("request") rather than
via the first "out of bounds" index of "channels", otherwise run-time
bounds checking will throw a warning.

Reported-by: Nathan Chancellor <nathan@kernel.org>
Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 net/wireless/nl80211.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f391b4055944..f1ed0981147e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -9163,6 +9163,7 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 	struct wiphy *wiphy;
 	int err, tmp, n_ssids = 0, n_channels, i;
 	size_t ie_len, size;
+	size_t ssids_offset, ie_offset;
 
 	wiphy = &rdev->wiphy;
 
@@ -9208,21 +9209,20 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 
 	size = struct_size(request, channels, n_channels);
+	ssids_offset = size;
 	size = size_add(size, array_size(sizeof(*request->ssids), n_ssids));
+	ie_offset = size;
 	size = size_add(size, ie_len);
 	request = kzalloc(size, GFP_KERNEL);
 	if (!request)
 		return -ENOMEM;
+	request->n_channels = n_channels;
 
 	if (n_ssids)
-		request->ssids = (void *)&request->channels[n_channels];
+		request->ssids = (void *)request + ssids_offset;
 	request->n_ssids = n_ssids;
-	if (ie_len) {
-		if (n_ssids)
-			request->ie = (void *)(request->ssids + n_ssids);
-		else
-			request->ie = (void *)(request->channels + n_channels);
-	}
+	if (ie_len)
+		request->ie = (void *)request + ie_offset;
 
 	i = 0;
 	if (scan_freqs) {
-- 
2.34.1

Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Nathan Chancellor]

On Wed, Apr 24, 2024 at 03:01:01PM -0700, Kees Cook wrote:
> Before request->channels[] can be used, request->n_channels must be set.
> Additionally, address calculations for memory after the "channels" array
> need to be calculated from the allocation base ("request") rather than
> via the first "out of bounds" index of "channels", otherwise run-time
> bounds checking will throw a warning.
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
>  net/wireless/nl80211.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index f391b4055944..f1ed0981147e 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -9163,6 +9163,7 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
>  	struct wiphy *wiphy;
>  	int err, tmp, n_ssids = 0, n_channels, i;
>  	size_t ie_len, size;
> +	size_t ssids_offset, ie_offset;
>  
>  	wiphy = &rdev->wiphy;
>  
> @@ -9208,21 +9209,20 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
>  		return -EINVAL;
>  
>  	size = struct_size(request, channels, n_channels);
> +	ssids_offset = size;
>  	size = size_add(size, array_size(sizeof(*request->ssids), n_ssids));
> +	ie_offset = size;
>  	size = size_add(size, ie_len);
>  	request = kzalloc(size, GFP_KERNEL);
>  	if (!request)
>  		return -ENOMEM;
> +	request->n_channels = n_channels;
>  
>  	if (n_ssids)
> -		request->ssids = (void *)&request->channels[n_channels];
> +		request->ssids = (void *)request + ssids_offset;
>  	request->n_ssids = n_ssids;
> -	if (ie_len) {
> -		if (n_ssids)
> -			request->ie = (void *)(request->ssids + n_ssids);
> -		else
> -			request->ie = (void *)(request->channels + n_channels);
> -	}
> +	if (ie_len)
> +		request->ie = (void *)request + ie_offset;
>  
>  	i = 0;
>  	if (scan_freqs) {
> -- 
> 2.34.1
> 

Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Johannes Berg]

On Wed, 2024-04-24 at 15:01 -0700, Kees Cook wrote:
> Before request->channels[] can be used, request->n_channels must be set.
> Additionally, address calculations for memory after the "channels" array
> need to be calculated from the allocation base ("request") rather than
> via the first "out of bounds" index of "channels", otherwise run-time
> bounds checking will throw a warning.
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")

While I was weighing whether or not to apply this for 6.9 still ...

> +	request->n_channels = n_channels;
>  
>  	if (n_ssids)
> -		request->ssids = (void *)&request->channels[n_channels];
> +		request->ssids = (void *)request + ssids_offset;

This really doesn't even seem right, shouldn't do pointer arithmetic on
void pointers. Same applies below too.

And also if you set n_channels before, perhaps it's actually OK to get a
pointer to *after*? Not sure though.

johannes

Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Jeff Johnson]

On 4/30/2024 3:01 AM, Johannes Berg wrote:
> This really doesn't even seem right, shouldn't do pointer arithmetic on
> void pointers.

FWIW I argued this in the past in another context and Linus gave his opinion:

https://lore.kernel.org/all/CAHk-=whFKYMrF6euVvziW+drw7-yi1pYdf=uccnzJ8k09DoTXA@mail.gmail.com/

Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Kees Cook]

On Tue, Apr 30, 2024 at 12:59:57PM -0700, Jeff Johnson wrote:
> On 4/30/2024 3:01 AM, Johannes Berg wrote:
> > This really doesn't even seem right, shouldn't do pointer arithmetic on
> > void pointers.
> 
> FWIW I argued this in the past in another context and Linus gave his opinion:
> 
> https://lore.kernel.org/all/CAHk-=whFKYMrF6euVvziW+drw7-yi1pYdf=uccnzJ8k09DoTXA@mail.gmail.com/

I was going to make the same argument. :) For this case, (void *) is
superior because we need to perform byte-granular arithmetic and we need
to use the implicit cast to the assigned variable's type.

The reason not to use the channels[] array is because we're not addressing
anything in the array -- we're addressing past it. Better to use the
correct allocation base.

-Kees

-- 
Kees Cook

Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Johannes Berg]

On Thu, 2024-04-25 at 11:13 -0700, Nathan Chancellor wrote:
> On Wed, Apr 24, 2024 at 03:01:01PM -0700, Kees Cook wrote:
> > Before request->channels[] can be used, request->n_channels must be set.
> > Additionally, address calculations for memory after the "channels" array
> > need to be calculated from the allocation base ("request") rather than
> > via the first "out of bounds" index of "channels", otherwise run-time
> > bounds checking will throw a warning.
> > 
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> 

How do you get this tested? We have the same, and more, bugs in
cfg80211_scan_6ghz() which I'm fixing right now, but no idea how to
actually get the checks done?

johannes

Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing

[Nathan Chancellor]

On Tue, May 07, 2024 at 12:46:46PM +0200, Johannes Berg wrote:
> On Thu, 2024-04-25 at 11:13 -0700, Nathan Chancellor wrote:
> > On Wed, Apr 24, 2024 at 03:01:01PM -0700, Kees Cook wrote:
> > > Before request->channels[] can be used, request->n_channels must be set.
> > > Additionally, address calculations for memory after the "channels" array
> > > need to be calculated from the allocation base ("request") rather than
> > > via the first "out of bounds" index of "channels", otherwise run-time
> > > bounds checking will throw a warning.
> > > 
> > > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > > Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > 
> > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > 
> 
> How do you get this tested? We have the same, and more, bugs in
> cfg80211_scan_6ghz() which I'm fixing right now, but no idea how to
> actually get the checks done?

You'll need a toolchain with __counted_by support, which I believe is
only clang 18+ at this point (I have prebuilts available at [1]), and
CONFIG_UBSAN_BOUNDS enabled, then they should just pop up in dmesg.

[1]: https://mirrors.edge.kernel.org/pub/tools/llvm/

Cheers,
Nathan