netdev - Re: [PATCH] ath9k: hif_usb: Fix use-after-free in ath9k_hif_usb_reg_in

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 07 Oct 2022 19:46:16 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Fedor Pchelkin <pchelkin@...ras.ru>, Kalle Valo <kvalo@...nel.org>
Cc:     Fedor Pchelkin <pchelkin@...ras.ru>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "John W. Linville" <linville@...driver.com>,
        Sujith Manoharan <Sujith.Manoharan@...eros.com>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Alexey Khoroshilov <khoroshilov@...ras.ru>,
        ldv-project@...uxtesting.org
Subject: Re: [PATCH] ath9k: hif_usb: Fix use-after-free in
 ath9k_hif_usb_reg_in_cb()

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