diff mbox series

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

Message ID 20240424220057.work.819-kees@kernel.org (mailing list archive)
State Mainlined
Commit 838c7b8f1f278404d9d684c34a8cb26dc41aaaa1
Headers show
Series wifi: nl80211: Avoid address calculations via out of bounds array indexing | expand

Commit Message

Kees Cook April 24, 2024, 10:01 p.m. UTC
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(-)

Comments

Nathan Chancellor April 25, 2024, 6:13 p.m. UTC | #1
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
>
Johannes Berg April 30, 2024, 10:01 a.m. UTC | #2
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
Jeff Johnson April 30, 2024, 7:59 p.m. UTC | #3
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/
Kees Cook April 30, 2024, 9 p.m. UTC | #4
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
Johannes Berg May 7, 2024, 10:46 a.m. UTC | #5
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
Nathan Chancellor May 7, 2024, 2:21 p.m. UTC | #6
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
diff mbox series

Patch

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) {