Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()

From: Johannes Berg <johannes@sipsolutions.net>
To: Jakub Kicinski <kuba@kernel.org>, Siddh Raman Pant <code@siddh.me>
Cc: eric dumazet <edumazet@google.com>,
	netdev <netdev@vger.kernel.org>, paolo abeni <pabeni@redhat.com>,
	linux-kernel-mentees
	<linux-kernel-mentees@lists.linuxfoundation.org>,
	"david s. miller" <davem@davemloft.net>
Subject: Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
Date: Mon, 15 Aug 2022 21:58:24 +0200	[thread overview]
Message-ID: <18fd9b89d45aedc1504d0cbd299ffb289ae96438.camel@sipsolutions.net> (raw)
In-Reply-To: <20220815094722.3c275087@kernel.org>

On Mon, 2022-08-15 at 09:47 -0700, Jakub Kicinski wrote:
> On Sat, 13 Aug 2022 21:49:52 +0530 Siddh Raman Pant wrote:
> > On Sat, 13 Aug 2022 00:55:09 +0530  Jakub Kicinski  wrote:
> > > Similarly to Greg, I'm not very familiar with the code base but one
> > > sure way to move things forward would be to point out a commit which
> > > broke things and put it in a Fixes tag. Much easier to validate a fix
> > > by looking at where things went wrong.  
> > 
> > Thanks, I now looked at some history.
> > 
> > The following commit on 28 Sep 2020 put the kfree call before NULLing:
> > c8cb5b854b40 ("nl80211/cfg80211: support 6 GHz scanning")
> > 
> > The following commit on 19 Nov 2014 introduces RCU:
> > 6ea0a69ca21b ("mac80211: rcu-ify scan and scheduled scan request pointers")
> > 
> > The kfree call wasn't "rcu-ified" in this commit, and neither were
> > RCU heads added.
> > 
> > The following commit on 18 Dec 2014 added RCU head for sched_scan_req:
> > 31a60ed1e95a ("nl80211: Convert sched_scan_req pointer to RCU pointer")
> > 
> > It seems a similar thing might not have been done for scan_req, but I
> > could have also missed commits.
> > 
> > So what should go into the fixes tag, if any? Probably 6ea0a69ca21b?
> 
> That'd be my instinct, too. But do add the full history analysis 
> to the commit message.
> 
> > Also, I probably should use RCU_INIT_POINTER in this patch. Or should
> > I make a patch somewhat like 31a60ed1e95a?
> 
> Yeah, IDK, I'm confused on what the difference between rdev and local
> is. The crash site reads the pointer from local, so other of clearing
> the pointer on rdev should not matter there. Hopefully wireless folks
> can chime in on v3.

Sorry everyone, I always thought "this looks odd" and then never got
around to taking a closer look.

So yeah, I still think this looks odd - cfg80211 doesn't really know
anything about how mac80211 might be doing something with RCU to protect
the pointer.

The patch also leaves the NULL-ing in mac80211 (that is how we reach it)
broken wrt. the kfree_rcu() since it doesn't happen _before_, and the
pointer in rdev isn't how this is reached through RCU (it's not even
__rcu annotated).

I think it might be conceptually better, though not faster, to do
something like https://p.sipsolutions.net/1d23837f455dc4c2.txt which
ensures that from mac80211's POV it can no longer be reached before we
call cfg80211_scan_done().

Yeah, that's slower, but scanning is still a relatively infrequent (and
slow anyway) operation, and this way we can stick to "this is not used
once you call cfg80211_scan_done()" which just makes much more sense?

johannes
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees