| Message ID | 20221008114917.21404-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dd95f2239fc846795fc926787c3ae0ca701c9840 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] 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> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Fedor Pchelkin <pchelkin@ispras.ru> wrote: > 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> > 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. dd95f2239fc8 wifi: ath9k: hif_usb: Fix use-after-free in ath9k_hif_usb_reg_in_cb()
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index 4d9002a9d082..401b408cb7a4 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); @@ -735,25 +734,27 @@ static void ath9k_hif_usb_reg_in_cb(struct urb *urb) if (likely(urb->actual_length != 0)) { skb_put(skb, urb->actual_length); - /* Process the command first */ + /* + * Process the command first. + * skb is either freed here or passed to be + * managed to another callback function. + */ ath9k_htc_rx_msg(hif_dev->htc_handle, skb, 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 +763,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> --- v1->v2: add comment about freeing an skb and remove double blank line drivers/net/wireless/ath/ath9k/hif_usb.c | 28 +++++++++++++----------- 1 file changed, 15 insertions(+), 13 deletions(-)