Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing
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
> > > Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct
> > > cfg80211_scan_request with __counted_by")
> > > Signed-off-by: Kees Cook
> >
> > Tested-by: Nathan Chancellor
> >
>
> 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
Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing
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
> > Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request
> > with __counted_by")
> > Signed-off-by: Kees Cook
>
> Tested-by: Nathan Chancellor
>
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
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=uccnzj8k09do...@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
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=uccnzj8k09do...@mail.gmail.com/
Re: [PATCH] wifi: nl80211: Avoid address calculations via out of bounds array indexing
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
> 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 *)>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
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
> Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request
> with __counted_by")
> Signed-off-by: Kees Cook
Tested-by: Nathan Chancellor
> ---
> Cc: Johannes Berg
> Cc: "David S. Miller"
> Cc: Eric Dumazet
> Cc: Jakub Kicinski
> Cc: Paolo Abeni
> Cc: linux-wirel...@vger.kernel.org
> Cc: net...@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 = >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 *)>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
>
