Skip to content

Commit dd95f22

Browse files
pchelkin91kvalo
authored andcommitted
wifi: ath9k: hif_usb: Fix use-after-free in ath9k_hif_usb_reg_in_cb()
It is possible that skb is freed in ath9k_htc_rx_msg(), then usb_submit_urb() fails and we try to free skb again. It causes use-after-free bug. Moreover, if alloc_skb() fails, urb->context becomes NULL but rx_buf is not freed and there can be a memory leak. The patch removes unnecessary nskb and makes skb processing more clear: it is supposed that ath9k_htc_rx_msg() either frees old skb or passes its managing to another callback function. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: 3deff76 ("ath9k_htc: Increase URB count for REG_IN pipe") Signed-off-by: Fedor Pchelkin <[email protected]> Signed-off-by: Alexey Khoroshilov <[email protected]> Acked-by: Toke Høiland-Jørgensen <[email protected]> Signed-off-by: Kalle Valo <[email protected]> Link: https://lore.kernel.org/r/[email protected]
1 parent d174768 commit dd95f22

File tree

1 file changed

+15
-13
lines changed

1 file changed

+15
-13
lines changed

drivers/net/wireless/ath/ath9k/hif_usb.c

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -708,14 +708,13 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb)
708708
struct rx_buf *rx_buf = (struct rx_buf *)urb->context;
709709
struct hif_device_usb *hif_dev = rx_buf->hif_dev;
710710
struct sk_buff *skb = rx_buf->skb;
711-
struct sk_buff *nskb;
712711
int ret;
713712

714713
if (!skb)
715714
return;
716715

717716
if (!hif_dev)
718-
goto free;
717+
goto free_skb;
719718

720719
switch (urb->status) {
721720
case 0:
@@ -724,7 +723,7 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb)
724723
case -ECONNRESET:
725724
case -ENODEV:
726725
case -ESHUTDOWN:
727-
goto free;
726+
goto free_skb;
728727
default:
729728
skb_reset_tail_pointer(skb);
730729
skb_trim(skb, 0);
@@ -735,25 +734,27 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb)
735734
if (likely(urb->actual_length != 0)) {
736735
skb_put(skb, urb->actual_length);
737736

738-
/* Process the command first */
737+
/*
738+
* Process the command first.
739+
* skb is either freed here or passed to be
740+
* managed to another callback function.
741+
*/
739742
ath9k_htc_rx_msg(hif_dev->htc_handle, skb,
740743
skb->len, USB_REG_IN_PIPE);
741744

742-
743-
nskb = alloc_skb(MAX_REG_IN_BUF_SIZE, GFP_ATOMIC);
744-
if (!nskb) {
745+
skb = alloc_skb(MAX_REG_IN_BUF_SIZE, GFP_ATOMIC);
746+
if (!skb) {
745747
dev_err(&hif_dev->udev->dev,
746748
"ath9k_htc: REG_IN memory allocation failure\n");
747-
urb->context = NULL;
748-
return;
749+
goto free_rx_buf;
749750
}
750751

751-
rx_buf->skb = nskb;
752+
rx_buf->skb = skb;
752753

753754
usb_fill_int_urb(urb, hif_dev->udev,
754755
usb_rcvintpipe(hif_dev->udev,
755756
USB_REG_IN_PIPE),
756-
nskb->data, MAX_REG_IN_BUF_SIZE,
757+
skb->data, MAX_REG_IN_BUF_SIZE,
757758
ath9k_hif_usb_reg_in_cb, rx_buf, 1);
758759
}
759760

@@ -762,12 +763,13 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb)
762763
ret = usb_submit_urb(urb, GFP_ATOMIC);
763764
if (ret) {
764765
usb_unanchor_urb(urb);
765-
goto free;
766+
goto free_skb;
766767
}
767768

768769
return;
769-
free:
770+
free_skb:
770771
kfree_skb(skb);
772+
free_rx_buf:
771773
kfree(rx_buf);
772774
urb->context = NULL;
773775
}

0 commit comments

Comments
 (0)