Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To:     Fedor Pchelkin <pchelkin@ispras.ru>, Kalle Valo <kvalo@kernel.org>
Cc:     Fedor Pchelkin <pchelkin@ispras.ru>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "John W. Linville" <linville@tuxdriver.com>,
        Sujith Manoharan <Sujith.Manoharan@atheros.com>,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        ldv-project@linuxtesting.org
Subject: Re: [PATCH] ath9k: hif_usb: Fix use-after-free in
 ath9k_hif_usb_reg_in_cb()
In-Reply-To: <20220728162149.212306-1-pchelkin@ispras.ru>
References: <20220728162149.212306-1-pchelkin@ispras.ru>
Date:   Fri, 07 Oct 2022 19:46:16 +0200
Message-ID: <87ilkvcys7.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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