wifi: nl80211: Avoid address calculations via out of bounds array indexing
Subsystems (3)
PATCH 1/1 wifi: nl80211: Avoid address calculations via out of bounds array indexing
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.
---
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(-)
net/wireless/nl80211.c +7 -7
@@ -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) {
Signature · 1 line
2.34.1
Quoted 9 lines · Kees Cook
Before request->channels[] can be used,
request->n_channelsmust 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]>
Quoted 55 lines
---
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(-)net/wireless/nl80211.c +7
-7@@ -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) {Signature · 1 line
2.34.1
Johannes Berg
maintainerQuoted 8 lines · Kees Cook
Before request->channels[] can be used,
request->n_channelsmust 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 ...
net/wireless/nl80211.c static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info) · line 9222
Quoted 5 lines
net/wireless/nl80211.c +2
-1+ 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
Quoted 2 lines · Johannes Berg
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/
Quoted 7 lines · Jeff Johnson
Quoted 2 lines · Johannes Berg
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
Signature · 1 line
Kees Cook
Johannes Berg
maintainerQuoted 7 lines · Nathan Chancellor
Quoted 6 lines · Kees Cook
Before request->channels[] can be used,
request->n_channelsmust 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.
Quoted 6 lines
Quoted 3 lines
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
Quoted 12 lines · Johannes Berg
Quoted 11 lines · Nathan Chancellor
Quoted 9 lines · Kees Cook
Before request->channels[] can be used,
request->n_channelsmust 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]>
Quoted 6 lines
Quoted 2 lines
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?
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