| Message ID | 20220202130510.GA27283@kili (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: fix use after free in rtw_hw_scan_update_probe_req() | expand |
On Wed, 2022-02-02 at 16:05 +0300, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> > --- > drivers/net/wireless/realtek/rtw88/fw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c > index ce9535cce723..2de0bb67bac6 100644 > --- a/drivers/net/wireless/realtek/rtw88/fw.c > +++ b/drivers/net/wireless/realtek/rtw88/fw.c > @@ -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; 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? --- a/drivers/net/wireless/realtek/rtw88/fw.c +++ b/drivers/net/wireless/realtek/rtw88/fw.c @@ -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
On Thu, Feb 03, 2022 at 07:59:47AM +0000, Pkshih wrote: > On Wed, 2022-02-02 at 16:05 +0300, Dan Carpenter wrote: > > 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 <dan.carpenter@oracle.com> > > --- > > drivers/net/wireless/realtek/rtw88/fw.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c > > index ce9535cce723..2de0bb67bac6 100644 > > --- a/drivers/net/wireless/realtek/rtw88/fw.c > > +++ b/drivers/net/wireless/realtek/rtw88/fw.c > > @@ -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; > > > 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
diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c index ce9535cce723..2de0bb67bac6 100644 --- a/drivers/net/wireless/realtek/rtw88/fw.c +++ b/drivers/net/wireless/realtek/rtw88/fw.c @@ -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;
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 <dan.carpenter@oracle.com> --- drivers/net/wireless/realtek/rtw88/fw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)