netdev - Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan

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-next>] [day] [month] [year] [list]

Message-ID: <18fd9b89d45aedc1504d0cbd299ffb289ae96438.camel@sipsolutions.net>
Date:   Mon, 15 Aug 2022 21:58:24 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, Siddh Raman Pant <code@...dh.me>
Cc:     greg kh <gregkh@...uxfoundation.org>,
        "david s. miller" <davem@...emloft.net>,
        eric dumazet <edumazet@...gle.com>,
        paolo abeni <pabeni@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        linux-kernel-mentees 
        <linux-kernel-mentees@...ts.linuxfoundation.org>
Subject: Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()

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