Re: [PATCH] ath9k: hif_usb: Fix use-after-free in ath9k_hif_usb_reg_in_cb()

[Toke Høiland-Jørgensen]

Fedor Pchelkin <pchelkin@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> ---
>  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