| Message ID | 20220728162149.212306-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Toke Høiland-Jørgensen |
| Headers | show |
| Series | ath9k: hif_usb: Fix use-after-free in ath9k_hif_usb_reg_in_cb() | expand |
Fedor Pchelkin <pchelkin@ispras.ru> writes: > 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: 3deff76095c4 ("ath9k_htc: Increase URB count for REG_IN pipe") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > drivers/net/wireless/ath/ath9k/hif_usb.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > index 518deb5098a2..b70128d1594d 100644 > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > @@ -708,14 +708,13 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) > struct rx_buf *rx_buf = (struct rx_buf *)urb->context; > struct hif_device_usb *hif_dev = rx_buf->hif_dev; > struct sk_buff *skb = rx_buf->skb; > - struct sk_buff *nskb; > int ret; > > if (!skb) > return; > > if (!hif_dev) > - goto free; > + goto free_skb; > > switch (urb->status) { > case 0: > @@ -724,7 +723,7 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) > case -ECONNRESET: > case -ENODEV: > case -ESHUTDOWN: > - goto free; > + goto free_skb; > default: > skb_reset_tail_pointer(skb); > skb_trim(skb, 0); > @@ -740,20 +739,19 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) > skb->len, USB_REG_IN_PIPE); > > > - nskb = alloc_skb(MAX_REG_IN_BUF_SIZE, GFP_ATOMIC); > - if (!nskb) { > + skb = alloc_skb(MAX_REG_IN_BUF_SIZE, GFP_ATOMIC); > + if (!skb) { The fix LGTM, but could you please add a comment here stating that the ath9k_htc_rx_msg() call above frees the skb (either here, or as part of the comment above the call to that function)? Also, please get rid of the double blank line while you're fixing things up... -Toke
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index 518deb5098a2..b70128d1594d 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -708,14 +708,13 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) struct rx_buf *rx_buf = (struct rx_buf *)urb->context; struct hif_device_usb *hif_dev = rx_buf->hif_dev; struct sk_buff *skb = rx_buf->skb; - struct sk_buff *nskb; int ret; if (!skb) return; if (!hif_dev) - goto free; + goto free_skb; switch (urb->status) { case 0: @@ -724,7 +723,7 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) case -ECONNRESET: case -ENODEV: case -ESHUTDOWN: - goto free; + goto free_skb; default: skb_reset_tail_pointer(skb); skb_trim(skb, 0); @@ -740,20 +739,19 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) skb->len, USB_REG_IN_PIPE); - nskb = alloc_skb(MAX_REG_IN_BUF_SIZE, GFP_ATOMIC); - if (!nskb) { + skb = alloc_skb(MAX_REG_IN_BUF_SIZE, GFP_ATOMIC); + if (!skb) { dev_err(&hif_dev->udev->dev, "ath9k_htc: REG_IN memory allocation failure\n"); - urb->context = NULL; - return; + goto free_rx_buf; } - rx_buf->skb = nskb; + rx_buf->skb = skb; usb_fill_int_urb(urb, hif_dev->udev, usb_rcvintpipe(hif_dev->udev, USB_REG_IN_PIPE), - nskb->data, MAX_REG_IN_BUF_SIZE, + skb->data, MAX_REG_IN_BUF_SIZE, ath9k_hif_usb_reg_in_cb, rx_buf, 1); } @@ -762,12 +760,13 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) ret = usb_submit_urb(urb, GFP_ATOMIC); if (ret) { usb_unanchor_urb(urb); - goto free; + goto free_skb; } return; -free: +free_skb: kfree_skb(skb); +free_rx_buf: kfree(rx_buf); urb->context = NULL; }
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: 3deff76095c4 ("ath9k_htc: Increase URB count for REG_IN pipe") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/net/wireless/ath/ath9k/hif_usb.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)