Re: [PATCH v2] wifi: ath9k: hif_usb: clean up skbs if ath9k_hif_usb_rx_stream() fails
From: Toke Høiland-Jørgensen <toke@toke.dk>
Date: 2023-01-03 21:03:15
Also in:
linux-wireless, lkml
Fedor Pchelkin [off-list ref] writes:
quoted hunk ↗ jump to hunk
Syzkaller detected a memory leak of skbs in ath9k_hif_usb_rx_stream(). While processing skbs in ath9k_hif_usb_rx_stream(), the already allocated skbs in skb_pool are not freed if ath9k_hif_usb_rx_stream() fails. If we have an incorrect pkt_len or pkt_tag, the skb is dropped and all the associated skb_pool buffers should be cleaned, too. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: 6ce708f54cc8 ("ath9k: Fix out-of-bound memcpy in ath9k_hif_usb_rx_stream") Fixes: 44b23b488d44 ("ath9k: hif_usb: Reduce indent 1 column") Reported-by: syzbot+e9632e3eb038d93d6bc6@syzkaller.appspotmail.com Signed-off-by: Fedor Pchelkin <redacted> Signed-off-by: Alexey Khoroshilov <redacted> --- v1->v2: added Reported-by tag drivers/net/wireless/ath/ath9k/hif_usb.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index 1a2e0c7eeb02..d02cec114280 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c@@ -586,14 +586,14 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, if (pkt_tag != ATH_USB_RX_STREAM_MODE_TAG) { RX_STAT_INC(hif_dev, skb_dropped); - return; + goto invalid_pkt; } if (pkt_len > 2 * MAX_RX_BUF_SIZE) { dev_err(&hif_dev->udev->dev, "ath9k_htc: invalid pkt_len (%x)\n", pkt_len); RX_STAT_INC(hif_dev, skb_dropped); - return; + goto invalid_pkt; } pad_len = 4 - (pkt_len & 0x3);@@ -654,6 +654,11 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, skb_pool[i]->len, USB_WLAN_RX_PIPE); RX_STAT_INC(hif_dev, skb_completed); } + return; +invalid_pkt: + for (i = 0; i < pool_index; i++) + kfree_skb(skb_pool[i]); + return;
Hmm, so in the other error cases (if SKB allocation fails), we just 'goto err' and call the receive handler for the packets already in skb_pool. Why can't we do the same here? Also, I think there's another bug in that function, which this change will make worse? Specifically, in the start of that function, hif_dev->remain_skb is moved to skb_pool[0], but not cleared from hif_dev itself. So if we then hit the invalid check and free it, the next time the function is called, we'll get the same remain_skb pointer, which has now been freed. So I think we'll need to clear out hif_dev->remain_skb after moving it to skb_pool. Care to add that as well? -Toke