| Message ID | 20240424220057.work.819-kees@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: nl80211: Avoid address calculations via out of bounds array indexing | expand |
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 >
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
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 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
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 --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) {
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(-)