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

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

[Siddh Raman Pant]

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@syzkaller.appspotmail.com
> Reported-by: syzbot+6cb476b7c69916a0caca@syzkaller.appspotmail.com
> Reported-by: syzbot+9250865a55539d384347@syzkaller.appspotmail.com
> 
> Signed-off-by: Siddh Raman Pant code@siddh.me>
> ---
> 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@siddh.me/

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?

Thanks,
Siddh

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@syzkaller.appspotmail.com
> > Reported-by: syzbot+6cb476b7c69916a0caca@syzkaller.appspotmail.com
> > Reported-by: syzbot+9250865a55539d384347@syzkaller.appspotmail.com
> > 
> > Signed-off-by: Siddh Raman Pant code@siddh.me>
> > ---
> > 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@siddh.me/
> 
> 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

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

[Siddh Raman Pant]

On Fri, 12 Aug 2022 17:45:58 +0530  Greg KH  wrote:
> 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.
> 

Okay, sure.

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

Why do you think so?

Thanks,
Siddh

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

[Greg KH]

On Fri, Aug 12, 2022 at 08:03:05PM +0530, Siddh Raman Pant wrote:
> On Fri, 12 Aug 2022 17:45:58 +0530  Greg KH  wrote:
> > 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.
> > 
> 
> Okay, sure.
> 
> > Personally, this patch seems very incorrect, but hey, I'm not the wifi
> > subsystem maintainer :)
> 
> Why do you think so?

rcu just delays freeing of an object, you might just be delaying the
race condition.  Just moving a single object to be freed with rcu feels
very odd if you don't have another reference somewhere.

Anyway, I don't know this codebase, so I could be totally wrong.

greg k-h

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

[Siddh Raman Pant]

On Fri, 12 Aug 2022 20:57:58 +0530  Greg KH  wrote:
> rcu just delays freeing of an object, you might just be delaying the
> race condition.  Just moving a single object to be freed with rcu feels
> very odd if you don't have another reference somewhere.

As mentioned in patch message, in net/mac80211/scan.c, we have:
        void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
        {
                ...
                scan_req = rcu_dereference(local->scan_req);
                sched_scan_req = rcu_dereference(local->sched_scan_req);

                if (scan_req)
                        scan_req_flags = scan_req->flags;
                ...
        }

So scan_req is probably supposed to be protected by RCU.

Also, in ieee80211_local's definition at net/mac80211/ieee80211_i.h, we have:
        struct cfg80211_scan_request __rcu *scan_req;

Thus, scan_req is indeed supposed to be protected by RCU, which this patch
addresses by adding a RCU head to the type's struct, and using kfree_rcu().

The above snippet is where the UAF happens (you can refer to syzkaller's log),
because __cfg80211_scan_done() is called and frees the pointer.

Thanks,
Siddh

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

[Jakub Kicinski]

On Fri, 12 Aug 2022 21:57:31 +0530 Siddh Raman Pant wrote:
> On Fri, 12 Aug 2022 20:57:58 +0530  Greg KH  wrote:
> > rcu just delays freeing of an object, you might just be delaying the
> > race condition.  Just moving a single object to be freed with rcu feels
> > very odd if you don't have another reference somewhere.  
> 
> As mentioned in patch message, in net/mac80211/scan.c, we have:
>         void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
>         {
>                 ...
>                 scan_req = rcu_dereference(local->scan_req);
>                 sched_scan_req = rcu_dereference(local->sched_scan_req);
> 
>                 if (scan_req)
>                         scan_req_flags = scan_req->flags;
>                 ...
>         }
> 
> So scan_req is probably supposed to be protected by RCU.
> 
> Also, in ieee80211_local's definition at net/mac80211/ieee80211_i.h, we have:
>         struct cfg80211_scan_request __rcu *scan_req;
> 
> Thus, scan_req is indeed supposed to be protected by RCU, which this patch
> addresses by adding a RCU head to the type's struct, and using kfree_rcu().
> 
> The above snippet is where the UAF happens (you can refer to syzkaller's log),
> because __cfg80211_scan_done() is called and frees the pointer.

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.

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

[Siddh Raman Pant]

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?

Also, I probably should use RCU_INIT_POINTER in this patch. Or should
I make a patch somewhat like 31a60ed1e95a?

Thanks,
Siddh