On Mon, 2025-07-14 at 22:04 -0700, Kees Cook wrote:
> On Mon, Jul 14, 2025 at 02:21:30PM +0200, Johannes Berg wrote:
> > If we really wanted to fix it, we'd need to separately track the
> > number of channels allocated and the number of channels currently
> > used, but given that no bugs were found despite the numerous syzbot
> > reports, that'd just be a waste of time.
>
> This mismatch between "quantity allocated" and "quantity used from the
> allocation" is repeated in more places that just wifi, and I'd agree
> that it has caused some confusion. The intent of __counted_by is to
> track the _allocation_, so my mistake was trying to apply it in places
> where the allocation size is not retained, and to shoe-horn it into the
> "used" tracking member.
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.
> Any opposition to adding such a field here, maybe "avail_channels"?
I guess fundamentally not, however, as described here, I don't think it
has actually all _that_ useful. We haven't found a single _real_ bug
with it in the two years this has been around, and quite frequently had
false positives. Now, tracking the allocation size would hopefully not
get into the false positives again but ...
Also, I still have 11 other such annotations that are probably at least
somewhat wrong in the same ways, and I guess it'll fall on me to review
them, since before forming an opinion on how it should be used I just
merged the changes, naively trusting you (and others.) The scheduled
scan for example is likely in a similar situation, though in that case
maybe we don't reuse the allocations as much.
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.
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);
};
which also helps for otherwise understanding how scratch_len is used.
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.
johannes
