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

[Greg KH]

On Fri, Aug 12, 2022 at 03:21:50PM +0530, Siddh Raman Pant via Linux-kernel-mentees wrote:
> On Tue, 26 Jul 2022 18:09:21 +0530  Siddh Raman Pant  wrote:
> > ieee80211_scan_rx() tries to access scan_req->flags after a null check
> > (see line 303 of mac80211/scan.c), but ___cfg80211_scan_done() uses
> > kfree() on the scan_req (see line 991 of wireless/scan.c).
> > 
> > This results in a UAF.
> > 
> > ieee80211_scan_rx() is called inside a RCU read-critical section
> > initiated by ieee80211_rx_napi() (see line 5044 of mac80211/rx.c).
> > 
> > Thus, add an rcu_head to the scan_req struct, so that we can use
> > kfree_rcu() instead of kfree() and thus not free during the critical
> > section.
> > 
> > We can clear the pointer before freeing here, since scan_req is
> > accessed using rcu_dereference().
> > 
> > Bug report (3): https://syzkaller.appspot.com/bug?extid=f9acff9bf08a845f225d
> > Reported-by: syzbot+f9acff9bf08a845f225d@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: syzbot+6cb476b7c69916a0caca@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: syzbot+9250865a55539d384347@xxxxxxxxxxxxxxxxxxxxxxxxx
> > 
> > Signed-off-by: Siddh Raman Pant code@xxxxxxxx>
> > ---
> > Changes since v1 as requested:
> > - Fixed commit heading and better commit message.
> > - Clear pointer before freeing.
> > 
> >  include/net/cfg80211.h | 2 ++
> >  net/wireless/scan.c    | 2 +-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > index 80f41446b1f0..7e0b448c4cdb 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -2368,6 +2368,7 @@ struct cfg80211_scan_6ghz_params {
> >   * @n_6ghz_params: number of 6 GHz params
> >   * @scan_6ghz_params: 6 GHz params
> >   * @bssid: BSSID to scan for (most commonly, the wildcard BSSID)
> > + * @rcu_head: (internal) RCU head to use for freeing
> >   */
> >  struct cfg80211_scan_request {
> >  	struct cfg80211_ssid *ssids;
> > @@ -2397,6 +2398,7 @@ struct cfg80211_scan_request {
> >  	bool scan_6ghz;
> >  	u32 n_6ghz_params;
> >  	struct cfg80211_scan_6ghz_params *scan_6ghz_params;
> > +	struct rcu_head rcu_head;
> >  
> >  	/* keep last */
> >  	struct ieee80211_channel *channels[];
> > diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> > index 6d82bd9eaf8c..6cf58fe6dea0 100644
> > --- a/net/wireless/scan.c
> > +++ b/net/wireless/scan.c
> > @@ -988,8 +988,8 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
> >  	kfree(rdev->int_scan_req);
> >  	rdev->int_scan_req = NULL;
> >  
> > -	kfree(rdev->scan_req);
> >  	rdev->scan_req = NULL;
> > +	kfree_rcu(rdev_req, rcu_head);
> >  
> >  	if (!send_message)
> >  		rdev->scan_msg = msg;
> > -- 
> > 2.35.1
> > 
> 
> Hello,
> 
> Probably the above quoted patch was missed, which can be found on
> https://lore.kernel.org/linux-wireless/20220726123921.29664-1-code@xxxxxxxx/
> 
> This patch was posted more than 2 weeks ago, with changes as requested.
> 
> With the merge window almost ending, may I request for another look at
> this patch?

The merge window is for new features to be added, bugfixes can be merged
at any point in time, but most maintainers close their trees until after
the merge window is closed before accepting new fixes, like this one.

So just relax, wait another week or so, and if there's no response,
resend it then.

Personally, this patch seems very incorrect, but hey, I'm not the wifi
subsystem maintainer :)

thanks,

greg k-h