rtw88: fix use after free in rtw_hw_scan_update_probe_req()

PATCH Superseded Dan Carpenter 2 participants 3 messages Feb 2, 2022 – Feb 3, 2022

Subsystems (2)

Kernel janitors Realtek wireless driver (rtw88)

lore

Share & tools

lore

Dan Carpenter

author

2022-02-02 13:05 UTC #0 lore raw

PATCH 1/1 rtw88: fix use after free in rtw_hw_scan_update_probe_req()

This code needs to use skb_queue_walk_safe() instead of skb_queue_walk() because it frees the list iterator.

Fixes d95984b5580d ("rtw88: fix memory overrun and memory leak during hw_scan")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/net/wireless/realtek/rtw88/fw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/wireless/realtek/rtw88/fw.c +2 -2

index ce9535cce723..2de0bb67bac6 100644
@@ -1864,7 +1864,7 @@ static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev,
 {
 	struct cfg80211_scan_request *req = rtwvif->scan_req;
 	struct sk_buff_head list;
-	struct sk_buff *skb;
+	struct sk_buff *skb, *tmp;
 	u8 num = req->n_ssids, i, bands = 0;
 	int ret;

@@ -1889,7 +1889,7 @@ static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev,
 	return _rtw_hw_scan_update_probe_req(rtwdev, num * bands, &list);
 
 out:
-	skb_queue_walk(&list, skb)
+	skb_queue_walk_safe(&list, skb, tmp)
 		kfree_skb(skb);
 
 	return ret;

Signature · 1 line

2.20.1

Pkshih

maintainer

2022-02-03 08:00 UTC #1 lore raw

Quoted 23 lines · Dan Carpenter

This code needs to use skb_queue_walk_safe() instead of skb_queue_walk() because it frees the list iterator.
Fixes d95984b5580d ("rtw88: fix memory overrun and memory leak during hw_scan")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/realtek/rtw88/fw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/realtek/rtw88/fw.c +1 -1
index ce9535cce723..2de0bb67bac6 100644
@@ -1864,7 +1864,7 @@ static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev,
 {
 	struct cfg80211_scan_request *req = rtwvif->scan_req;
 	struct sk_buff_head list;
-	struct sk_buff *skb;
+	struct sk_buff *skb, *tmp;
 	u8 num = req->n_ssids, i, bands = 0;
 	int ret;
 

drivers/net/wireless/realtek/rtw88/fw.c static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev, · line 1895

Quoted 8 lines

drivers/net/wireless/realtek/rtw88/fw.c +1 -1

@@ -1889,7 +1889,7 @@ static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev,
 	return _rtw_hw_scan_update_probe_req(rtwdev, num * bands, &list);
 
 out:
-	skb_queue_walk(&list, skb)
+	skb_queue_walk_safe(&list, skb, tmp)
 		kfree_skb(skb);
 
 	return ret;

Oops, when I reivewed the patch "rtw88: fix memory overrun and memory leak during hw_scan", I did only focus on pointers of list head, but forget skb is freed that leads use after free.

Could I have related fix with this patch?

drivers/net/wireless/realtek/rtw88/fw.c +0 -0

@@ -1853,7 +1853,7 @@ static int _rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev, u8

num_probes,

        rtwdev->scan_info.probe_pg_size = page_offset;
 out:
        kfree(buf);
-       skb_queue_walk(probe_req_list, skb)
+       skb_queue_walk_safe(probe_req_list, skb, tmp)
                kfree_skb(skb);
 
        return ret;

--
Ping-Ke

Dan Carpenter

author

2022-02-03 08:22 UTC #2 lore raw

Quoted 31 lines · Pkshih

Quoted 30 lines · Dan Carpenter

This code needs to use skb_queue_walk_safe() instead of skb_queue_walk() because it frees the list iterator.

Fixes d95984b5580d ("rtw88: fix memory overrun and memory leak during hw_scan")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/net/wireless/realtek/rtw88/fw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/wireless/realtek/rtw88/fw.c +2 -2

index ce9535cce723..2de0bb67bac6 100644
@@ -1864,7 +1864,7 @@ static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev,
 {
 	struct cfg80211_scan_request *req = rtwvif->scan_req;
 	struct sk_buff_head list;
-	struct sk_buff *skb;
+	struct sk_buff *skb, *tmp;
 	u8 num = req->n_ssids, i, bands = 0;
 	int ret;

@@ -1889,7 +1889,7 @@ static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev,
 	return _rtw_hw_scan_update_probe_req(rtwdev, num * bands, &list);
 
 out:
-	skb_queue_walk(&list, skb)
+	skb_queue_walk_safe(&list, skb, tmp)
 		kfree_skb(skb);

Quoted 8 lines

drivers/net/wireless/realtek/rtw88/fw.c static int rtw_hw_scan_update_probe_req(struct rtw_dev *rtwdev, · line 1895
Quoted 1 line
drivers/net/wireless/realtek/rtw88/fw.c +0 -0
 	return ret;
Oops, when I reivewed the patch "rtw88: fix memory overrun and memory leak during hw_scan", I did only focus on pointers of list head, but forget skb is freed that leads use after free.
Could I have related fix with this patch?

Yes, thank you for noticing that. For some reason, on my system, Smatch thinks that probe_req_list is always empty and doesn't warn about that one because "it's impossible".

I will send a v2 patch.

regards,
dan carpenter