| Message ID | 20220726123921.29664-1-code@siddh.me (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] wifi: cfg80211: Fix UAF in ieee80211_scan_rx() | expand |
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
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
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
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
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
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.
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
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
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
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;
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(-)