Re: [PATCH wireless] wifi: cfg80211: remove scan request n_channels counted_by

[Kees Cook <kees@kernel.org>]

On Tue, Jul 15, 2025 at 10:24:16AM +0200, Johannes Berg wrote:
> I'd expect in most cases it's really the same - you allocate, fill, and
> never touch it again before throwing it away at the end. I'd argue
> though that in those cases the whole thing is quite pointless, although
> it still allows finding out-of-bounds reads.

Adding counted_by has filled a large gap in the compiler's ability to
provide the bounds checking. Back in 2022 I had mostly written it off as
"unsolvable" with dynamic sizes being over 60% of the uninstrumentable
memcpy() destinations: https://outflux.net/slides/2022/lss-na/#/6

But thanks to counted_by, we can actually chip away at that percentage
and it's fallen under 50% now, which is nice. :)

> So for now: no, I'm not going to apply any new counted_by() annotations.
> It's cost me far too much time for having absolutely nothing to show for
> the investment. Ask me again again next year maybe.

Yup, totally understood. My desire to get them applied was mainly due to
there being so many (fixed size) array bounds problems in various wifi
drivers that it felt like a good place to focus: dynamic sizes in the
core wifi code. From the same slides above, you can arrow-down through
all of the then-recent array bounds flaws and excepting one in BT,
they're all in wifi: https://outflux.net/slides/2022/lss-na/#/1/6

> If you feel motivated you could help review and perhaps annotate the
> still existing ones I guess, I'm thinking we should have comments there
> like this perhaps:
> 
> --- a/net/mac80211/parse.c
> +++ b/net/mac80211/parse.c
> @@ -54,8 +54,9 @@ struct ieee80211_elems_parse {
>          * scratch buffer that can be used for various element parsing related
>          * tasks, e.g., element de-fragmentation etc.
>          */
> -       size_t scratch_len;
>         u8 *scratch_pos;
> +       /* __counted_by: scratch_len tracks the allocation and doesn't change */
> +       size_t scratch_len;
>         u8 scratch[] __counted_by(scratch_len);
>  };

Sure, I can add this to the TODO list.

> But I'm also not completely sure I've convinced myself that all the
> above discussion about allocated vs. used is really the _entire_
> explanation for it being such a spectacular failure here.

I think it's a big part of it -- having the counted member change after
initial assignment is even frowned upon by the compiler folks, but is
technically supported.

Anyway, sorry again for the wasting of time of yours (and others) that I
caused with this -- I really wasn't expecting it to go that way, and it
hasn't been anywhere near as troublesome in other areas of the kernel,
so it took me by surprise. I have tried to chase down and fix the glitches
when I became aware of them, FWIW.

I'll see if I can write up some patches for comments like you suggest
above with good "proof" attached to them. :)

-Kees

-- 
Kees Cook