linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
@ 2022-07-26 12:39 Siddh Raman Pant via Linux-kernel-mentees
  2022-08-12  9:51 ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 12+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-07-26 12:39 UTC (permalink / raw)
  To: Johannes Berg, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, syzbot+6cb476b7c69916a0caca, linux-wireless,
	syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347,
	linux-kernel-mentees

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


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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-07-26 12:39 [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-12  9:51 ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-12 12:15   ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-12  9:51 UTC (permalink / raw)
  To: Johannes Berg, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, syzbot+6cb476b7c69916a0caca, linux-wireless,
	linux-kernel, syzbot+f9acff9bf08a845f225d,
	syzbot+9250865a55539d384347, linux-kernel-mentees

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-12  9:51 ` Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-12 12:15   ` Greg KH
  2022-08-12 14:33     ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2022-08-12 12:15 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: netdev, syzbot+6cb476b7c69916a0caca, linux-wireless,
	linux-kernel, syzbot+f9acff9bf08a845f225d, Eric Dumazet,
	syzbot+9250865a55539d384347, Jakub Kicinski, Johannes Berg,
	Paolo Abeni, linux-kernel-mentees, David S. Miller

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-12 12:15   ` Greg KH
@ 2022-08-12 14:33     ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-12 15:27       ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-12 14:33 UTC (permalink / raw)
  To: Greg KH
  Cc: netdev, syzbot+6cb476b7c69916a0caca, linux-wireless,
	linux-kernel, syzbot+f9acff9bf08a845f225d, eric dumazet,
	syzbot+9250865a55539d384347, jakub kicinski, johannes berg,
	paolo abeni, linux-kernel-mentees, david s. miller

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-12 14:33     ` Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-12 15:27       ` Greg KH
  2022-08-12 16:27         ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2022-08-12 15:27 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: netdev, syzbot+6cb476b7c69916a0caca, linux-wireless,
	linux-kernel, syzbot+f9acff9bf08a845f225d, eric dumazet,
	syzbot+9250865a55539d384347, jakub kicinski, johannes berg,
	paolo abeni, linux-kernel-mentees, david s. miller

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-12 15:27       ` Greg KH
@ 2022-08-12 16:27         ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-12 19:25           ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-12 16:27 UTC (permalink / raw)
  To: Greg KH
  Cc: netdev, syzbot+6cb476b7c69916a0caca, linux-wireless,
	linux-kernel, syzbot+f9acff9bf08a845f225d, eric dumazet,
	syzbot+9250865a55539d384347, jakub kicinski, johannes berg,
	paolo abeni, linux-kernel-mentees, david s. miller

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-12 16:27         ` Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-12 19:25           ` Jakub Kicinski
  2022-08-13 16:19             ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2022-08-12 19:25 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: syzbot+9250865a55539d384347, syzbot+6cb476b7c69916a0caca,
	linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d,
	eric dumazet, netdev, johannes berg, paolo abeni,
	linux-kernel-mentees, david s. miller

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.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-12 19:25           ` Jakub Kicinski
@ 2022-08-13 16:19             ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-15 16:47               ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-13 16:19 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: syzbot+9250865a55539d384347, syzbot+6cb476b7c69916a0caca,
	linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d,
	eric dumazet, netdev, johannes berg, paolo abeni,
	linux-kernel-mentees, david s. miller

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-13 16:19             ` Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-15 16:47               ` Jakub Kicinski
  2022-08-15 19:58                 ` Johannes Berg
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2022-08-15 16:47 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: syzbot+6cb476b7c69916a0caca,
	         
	<syzbot+6cb476b7c69916a0caca@syzkaller.appspotmail.com>, ,
	eric  dumazet <edumazet@google.com>,
	netdev <netdev@vger.kernel.org>,
	 johannes berg <johannes@sipsolutions.net>,
	paolo abeni <pabeni@redhat.com>,  ,
	david, s.miller,  <davem@davemloft.net>

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.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-15 16:47               ` Jakub Kicinski
@ 2022-08-15 19:58                 ` Johannes Berg
  2022-08-16  0:51                   ` Jakub Kicinski
  2022-08-16  8:52                   ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 2 replies; 12+ messages in thread
From: Johannes Berg @ 2022-08-15 19:58 UTC (permalink / raw)
  To: Jakub Kicinski, Siddh Raman Pant
  Cc: eric dumazet, netdev, paolo abeni, linux-kernel-mentees, david s. miller

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-15 19:58                 ` Johannes Berg
@ 2022-08-16  0:51                   ` Jakub Kicinski
  2022-08-16  8:52                   ` Siddh Raman Pant via Linux-kernel-mentees
  1 sibling, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2022-08-16  0:51 UTC (permalink / raw)
  To: Johannes Berg
  Cc: david s. miller"  <davem@davemloft.net>,
	eric dumazet  <edumazet@google.com>,
	paolo abeni  <pabeni@redhat.com>,
	netdev  <netdev@vger.kernel.org>,
	linux-kernel-mentees
	<linux-kernel-mentees@lists.linuxfoundation.org>

On Mon, 15 Aug 2022 21:58:24 +0200 Johannes Berg wrote:
> 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?

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx()
  2022-08-15 19:58                 ` Johannes Berg
  2022-08-16  0:51                   ` Jakub Kicinski
@ 2022-08-16  8:52                   ` Siddh Raman Pant via Linux-kernel-mentees
  1 sibling, 0 replies; 12+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-16  8:52 UTC (permalink / raw)
  To: Johannes Berg
  Cc: netdev, eric dumazet, jakub kicinski, paolo abeni,
	linux-kernel-mentees, david s. miller

On Tue, 16 Aug 2022 01:28:24 +0530  Johannes Berg  wrote:
> 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).
> 

Thanks for the critical review.

> 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
> 

Agreed, that looks like a good way to go about.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-09-02 16:37 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-26 12:39 [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() Siddh Raman Pant via Linux-kernel-mentees
2022-08-12  9:51 ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-12 12:15   ` Greg KH
2022-08-12 14:33     ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-12 15:27       ` Greg KH
2022-08-12 16:27         ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-12 19:25           ` Jakub Kicinski
2022-08-13 16:19             ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-15 16:47               ` Jakub Kicinski
2022-08-15 19:58                 ` Johannes Berg
2022-08-16  0:51                   ` Jakub Kicinski
2022-08-16  8:52                   ` Siddh Raman Pant via Linux-kernel-mentees

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).