linux-hardening - Re: Review of __counted_by in wireless (was Re: [PATCH wireless] wifi: cfg80211: remove scan request n_channels counted

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3a0ad03a4dd4a201df0be07ebf9581a8efeedafc.camel@sipsolutions.net>
Date: Mon, 21 Jul 2025 20:52:48 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Kees Cook <kees@...nel.org>
Cc: linux-wireless@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: Review of __counted_by in wireless (was Re: [PATCH wireless]
 wifi: cfg80211: remove scan request n_channels counted_by)

On Mon, 2025-07-21 at 11:36 -0700, Kees Cook wrote:
> This seemed like a good project to compare my manual review against a
> review performed by the LLM I've been playing with, and it did okay. It
> missed several dynamic cases and I had to prod it into getting them
> right, and it couldn't find some allocation patterns on its own. It did
> spontaneously find the le/ge counted_by variants, which was nice. Anyway,
> here are the results...

Heh, cool, I'm glad you did that. Thanks.

[snip]

> In the review, I found 3 existing bugs, which I've set as separate
> patches now:
> https://lore.kernel.org/lkml/20250721181810.work.575-kees@kernel.org/
> https://lore.kernel.org/lkml/20250721182521.work.540-kees@kernel.org/
> https://lore.kernel.org/lkml/20250721183125.work.183-kees@kernel.org/

Right. I'm not sure I'll get those in for 6.16 still, I was really
hoping to not send more changes. I guess I'll decide today or tomorrow,
but I also haven't see any complaints about that.

> At the end of the day, with the above fixes, I think the dynamic cases
> appear safe, as they all follow basically the same pattern of allocating
> some max size and then filling/filtering the actual population of the
> array. (So I think cfg80211_scan_request is correctly used at this point,
> but I understand your desire to remove __counted_by on it.)

It's not correct, which is why I removed it now. In mac80211, we
allocate the int_scan_req and set the n_channels, but we can use that
request multiple times, and of course if e.g. the first use just fills a
single channel and the second use wants more than one, then it gets a
complaint from UBSAN although we allocated enough space for all the
channels the device actually has.

That case is something you can't really ever check though, it assumes
that channels are never going to be added twice and that the device
never changes the list of channels on the fly, which both semantically
makes sense so I don't think any code would ever really break it, but
it's definitely harder to reason about.

Or we could have n_chans_allocated which only makes it detect the memory
corruption part of this issue and can never do anything more than that
such as detection misuse during reading the channel list (which should
only go to n_chans_used.)

> Do you want me to send patches for the static cases to add comments or
> is that too much churn?

I may have changed my mind on that, it's easy to have a comment that
says "it never changes" but then new code comes along that does just
that and never notices the comment ... I guess I'm still not sure about
the whole thing after all this, it seems awfully hard to reason about in
many cases.

johannes