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 <[email protected]>
Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Signed-off-by: Kees Cook <[email protected]>
---
Cc: Johannes Berg <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
> 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
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/
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 <[email protected]>
> > Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
> > Signed-off-by: Kees Cook <[email protected]>
>
> Tested-by: Nathan Chancellor <[email protected]>
>
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