* [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-07-26 12:39 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 22+ messages in thread From: Siddh Raman Pant @ 2022-07-26 12:39 UTC (permalink / raw) To: Johannes Berg, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: linux-wireless, netdev, linux-kernel-mentees, syzbot+f9acff9bf08a845f225d, syzbot+6cb476b7c69916a0caca, syzbot+9250865a55539d384347 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-07-26 12:39 ` Siddh Raman Pant via Linux-kernel-mentees 0 siblings, 0 replies; 22+ 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] 22+ messages in thread
* Re: [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 -1 siblings, 0 replies; 22+ messages in thread From: Siddh Raman Pant @ 2022-08-12 9:51 UTC (permalink / raw) To: Johannes Berg, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: linux-wireless, netdev, linux-kernel-mentees, syzbot+f9acff9bf08a845f225d, syzbot+6cb476b7c69916a0caca, syzbot+9250865a55539d384347, linux-kernel 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 ^ permalink raw reply [flat|nested] 22+ 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 0 siblings, 0 replies; 22+ 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] 22+ 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 -1 siblings, 0 replies; 22+ messages in thread From: Greg KH @ 2022-08-12 12:15 UTC (permalink / raw) To: Siddh Raman Pant Cc: Johannes Berg, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, syzbot+6cb476b7c69916a0caca, linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-08-12 12:15 ` Greg KH 0 siblings, 0 replies; 22+ 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] 22+ 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 -1 siblings, 0 replies; 22+ messages in thread From: Siddh Raman Pant @ 2022-08-12 14:33 UTC (permalink / raw) To: Greg KH Cc: johannes berg, david s. miller, eric dumazet, jakub kicinski, paolo abeni, netdev, syzbot+6cb476b7c69916a0caca, linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ 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 0 siblings, 0 replies; 22+ 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] 22+ 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 -1 siblings, 0 replies; 22+ 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] 22+ messages in thread
* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-08-12 15:27 ` Greg KH 0 siblings, 0 replies; 22+ messages in thread From: Greg KH @ 2022-08-12 15:27 UTC (permalink / raw) To: Siddh Raman Pant Cc: johannes berg, david s. miller, eric dumazet, jakub kicinski, paolo abeni, netdev, syzbot+6cb476b7c69916a0caca, linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ 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 -1 siblings, 0 replies; 22+ messages in thread From: Siddh Raman Pant @ 2022-08-12 16:27 UTC (permalink / raw) To: Greg KH Cc: johannes berg, david s. miller, eric dumazet, jakub kicinski, paolo abeni, netdev, syzbot+6cb476b7c69916a0caca, linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ 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 0 siblings, 0 replies; 22+ 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] 22+ 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 -1 siblings, 0 replies; 22+ messages in thread From: Jakub Kicinski @ 2022-08-12 19:25 UTC (permalink / raw) To: Siddh Raman Pant Cc: Greg KH, johannes berg, david s. miller, eric dumazet, paolo abeni, netdev, syzbot+6cb476b7c69916a0caca, linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347, linux-kernel-mentees 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-08-12 19:25 ` Jakub Kicinski 0 siblings, 0 replies; 22+ 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] 22+ 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 -1 siblings, 0 replies; 22+ messages in thread From: Siddh Raman Pant @ 2022-08-13 16:19 UTC (permalink / raw) To: Jakub Kicinski Cc: greg kh, johannes berg, david s. miller, eric dumazet, paolo abeni, netdev, syzbot+6cb476b7c69916a0caca, linux-wireless, linux-kernel, syzbot+f9acff9bf08a845f225d, syzbot+9250865a55539d384347, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ 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 0 siblings, 0 replies; 22+ 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] 22+ 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 -1 siblings, 1 reply; 22+ 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] 22+ 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 0 siblings, 0 replies; 22+ 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] 22+ messages in thread
* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-08-15 19:58 ` Johannes Berg 0 siblings, 0 replies; 22+ messages in thread From: Johannes Berg @ 2022-08-15 19:58 UTC (permalink / raw) To: Jakub Kicinski, Siddh Raman Pant Cc: greg kh, david s. miller, eric dumazet, paolo abeni, netdev, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ 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 -1 siblings, 0 replies; 22+ 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] 22+ messages in thread
* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() 2022-08-15 19:58 ` Johannes Berg @ 2022-08-16 8:52 ` Siddh Raman Pant -1 siblings, 0 replies; 22+ 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] 22+ messages in thread
* Re: [PATCH v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() @ 2022-08-16 8:52 ` Siddh Raman Pant 0 siblings, 0 replies; 22+ messages in thread From: Siddh Raman Pant @ 2022-08-16 8:52 UTC (permalink / raw) To: Johannes Berg Cc: jakub kicinski, greg kh, david s. miller, eric dumazet, paolo abeni, netdev, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-09-02 16:37 UTC | newest] Thread overview: 22+ 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 2022-07-26 12:39 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-12 9:51 ` Siddh Raman Pant 2022-08-12 9:51 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-12 12:15 ` Greg KH 2022-08-12 12:15 ` Greg KH 2022-08-12 14:33 ` Siddh Raman Pant 2022-08-12 14:33 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-12 15:27 ` Greg KH 2022-08-12 15:27 ` Greg KH 2022-08-12 16:27 ` Siddh Raman Pant 2022-08-12 16:27 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-12 19:25 ` Jakub Kicinski 2022-08-12 19:25 ` Jakub Kicinski 2022-08-13 16:19 ` Siddh Raman Pant 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-15 19:58 ` Johannes Berg 2022-08-16 0:51 ` Jakub Kicinski 2022-08-16 8:52 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-16 8:52 ` Siddh Raman Pant
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.