| Message ID | 20220721101018.17902-1-code@siddh.me (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESEND] net: Fix UAF in ieee80211_scan_rx() | expand |
On Thu, Jul 21, 2022 at 12:11 PM Siddh Raman Pant via Linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org> 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 5043 of mac80211/rx.c). > > Thus, add an rcu_head to the scan_req struct so as to use kfree_rcu() > instead of kfree() so that we don't free during the critical section. > > 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> > --- > Resending because didn't get any reply from maintainers for more > than 2 weeks. > Siddh, I had a look at the Bug report above. Currently, we do not have any syzkaller or C reproducer to confirm that the bug was actually fixed. Now, that you have a supposed fix for the issue: Can you write a 'stress test' and (qemu) setup script that eventually makes that bug trigger (e.g., if we run the stress test for two or three days it will eventually trigger)? Then, we can also use that to confirm that your patch fixes the issue (beyond the normal sanity code review). This is certainly something you can do on your side to move this patch forward, and other developers with testing infrastructure can pick up and confirm your tests and results independently. I hope this helps, otherwise you will just need to have some patience. Best regards, Lukas > 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 6d02e12e4702..ba4a49884de8 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..638b2805222c 100644 > --- a/net/wireless/scan.c > +++ b/net/wireless/scan.c > @@ -988,7 +988,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, > kfree(rdev->int_scan_req); > rdev->int_scan_req = NULL; > > - kfree(rdev->scan_req); > + kfree_rcu(rdev->scan_req, rcu_head); > rdev->scan_req = NULL; > > if (!send_message) > -- > 2.35.1 > > > _______________________________________________ > Linux-kernel-mentees mailing list > Linux-kernel-mentees@lists.linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Hello Lukas, Sorry for the late reply. On Thu, 21 Jul 2022 16:05:01 +0530 Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > Siddh, > > I had a look at the Bug report above. Currently, we do not have any > syzkaller or C reproducer to confirm that the bug was actually fixed. > > Now, that you have a supposed fix for the issue: > Can you write a 'stress test' and (qemu) setup script that eventually > makes that bug trigger (e.g., if we run the stress test for two or > three days it will eventually trigger)? Then, we can also use that to > confirm that your patch fixes the issue (beyond the normal sanity code > review). > > This is certainly something you can do on your side to move this patch > forward, and other developers with testing infrastructure can pick up > and confirm your tests and results independently. I have been intermittently looking about this for the past few days. Since such test creation is new to me, I am stuck at how to go about calling the requisite function. What I have gathered is that I need to use the netlink API or related tool and issue the scan and recieve commands. Since qemu by default doesn't have a WiFi interface setup by default, I was looking at simulation and came across the mac80211_hwsim module. After building the kernel it, I tried using `iw` command for scanning with the two phy simulated devices, but I seem to hit a deadend due to not being able to properly use them for the task at hand. Do you have any resources or/and examples on such "stress tests"? > I hope this helps, otherwise you will just need to have some patience. > > Best regards, > > Lukas Eric had replied to me on the original email soon after, and I have sent a v2. Though, I still want to see how people go about making the tests, so any resources for further exploring will be useful. Thanks, Siddh
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 6d02e12e4702..ba4a49884de8 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..638b2805222c 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -988,7 +988,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, kfree(rdev->int_scan_req); rdev->int_scan_req = NULL; - kfree(rdev->scan_req); + kfree_rcu(rdev->scan_req, rcu_head); rdev->scan_req = NULL; if (!send_message)
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 5043 of mac80211/rx.c). Thus, add an rcu_head to the scan_req struct so as to use kfree_rcu() instead of kfree() so that we don't free during the critical section. 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> --- Resending because didn't get any reply from maintainers for more than 2 weeks. include/net/cfg80211.h | 2 ++ net/wireless/scan.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)