| Message ID | 20230104123615.51511-1-pchelkin@ispras.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] wifi: ath9k: hif_usb: clean up skbs if ath9k_hif_usb_rx_stream() fails | expand |
Fedor Pchelkin <pchelkin@ispras.ru> writes: > 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 input skb is considered invalid > and dropped. All the associated packets already in skb_pool should be > dropped and freed. Added a comment describing this issue. > > The patch also makes remain_skb NULL after being processed so that it > cannot be referenced after potential free. The initialization of hif_dev > fields which are associated with remain_skb (rx_remain_len, > rx_transfer_len and rx_pad_len) is moved after a new remain_skb is > allocated. > > 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 <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Fedor Pchelkin <pchelkin@ispras.ru> wrote: > 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 input skb is considered invalid > and dropped. All the associated packets already in skb_pool should be > dropped and freed. Added a comment describing this issue. > > The patch also makes remain_skb NULL after being processed so that it > cannot be referenced after potential free. The initialization of hif_dev > fields which are associated with remain_skb (rx_remain_len, > rx_transfer_len and rx_pad_len) is moved after a new remain_skb is > allocated. > > 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 <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 0af54343a762 wifi: ath9k: hif_usb: clean up skbs if ath9k_hif_usb_rx_stream() fails
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index 1a2e0c7eeb02..de6c0824c9ca 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -561,11 +561,11 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, memcpy(ptr, skb->data, rx_remain_len); rx_pkt_len += rx_remain_len; - hif_dev->rx_remain_len = 0; skb_put(remain_skb, rx_pkt_len); skb_pool[pool_index++] = remain_skb; - + hif_dev->remain_skb = NULL; + hif_dev->rx_remain_len = 0; } else { index = rx_remain_len; } @@ -584,16 +584,21 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, pkt_len = get_unaligned_le16(ptr + index); pkt_tag = get_unaligned_le16(ptr + index + 2); + /* It is supposed that if we have an invalid pkt_tag or + * pkt_len then the whole input SKB is considered invalid + * and dropped; the associated packets already in skb_pool + * are dropped, too. + */ 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); @@ -605,11 +610,6 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, if (index > MAX_RX_BUF_SIZE) { spin_lock(&hif_dev->rx_lock); - hif_dev->rx_remain_len = index - MAX_RX_BUF_SIZE; - hif_dev->rx_transfer_len = - MAX_RX_BUF_SIZE - chk_idx - 4; - hif_dev->rx_pad_len = pad_len; - nskb = __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC); if (!nskb) { dev_err(&hif_dev->udev->dev, @@ -617,6 +617,12 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, spin_unlock(&hif_dev->rx_lock); goto err; } + + hif_dev->rx_remain_len = index - MAX_RX_BUF_SIZE; + hif_dev->rx_transfer_len = + MAX_RX_BUF_SIZE - chk_idx - 4; + hif_dev->rx_pad_len = pad_len; + skb_reserve(nskb, 32); RX_STAT_INC(hif_dev, skb_allocated); @@ -654,6 +660,13 @@ 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++) { + dev_kfree_skb_any(skb_pool[i]); + RX_STAT_INC(hif_dev, skb_dropped); + } + return; } static void ath9k_hif_usb_rx_cb(struct urb *urb)
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 input skb is considered invalid and dropped. All the associated packets already in skb_pool should be dropped and freed. Added a comment describing this issue. The patch also makes remain_skb NULL after being processed so that it cannot be referenced after potential free. The initialization of hif_dev fields which are associated with remain_skb (rx_remain_len, rx_transfer_len and rx_pad_len) is moved after a new remain_skb is allocated. 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 <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- v1->v2: added Reported-by tag v2->v3: added proper remain_skb processing, stat macro, comment drivers/net/wireless/ath/ath9k/hif_usb.c | 31 +++++++++++++++++------- 1 file changed, 22 insertions(+), 9 deletions(-)