Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs

[Toke Høiland-Jørgensen]

Fedor Pchelkin <pchelkin@xxxxxxxxx> writes:

> hif_dev->remain_skb is allocated and used exclusively in
> ath9k_hif_usb_rx_stream(). It is implied that an allocated remain_skb is
> processed and subsequently freed (in error paths) only during the next
> call of ath9k_hif_usb_rx_stream().
>
> So, if the device is deinitialized between those two calls or if the skb
> contents are incorrect, it is possible that ath9k_hif_usb_rx_stream() is
> not called next time and the allocated remain_skb is leaked. Our local
> Syzkaller instance was able to trigger that.
>
> Fix the leak by introducing a function to explicitly free remain_skb (if
> it is not NULL) when the device is being deinitialized. remain_skb is NULL
> when it has not been allocated at all (hif_dev struct is kzalloced) or
> when it has been proccesed in next call to ath9k_hif_usb_rx_stream().
>
> Proper spinlocks are held to prevent possible concurrent access to
> remain_skb from the interrupt context ath9k_hif_usb_rx_stream(). These
> accesses should not happen as rx_urbs have been deallocated before but
> it prevents a dangerous race condition in these cases.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> ---
>  drivers/net/wireless/ath/ath9k/hif_usb.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index f521dfa2f194..e03ab972edf7 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -534,6 +534,23 @@ static struct ath9k_htc_hif hif_usb = {
>  	.send = hif_usb_send,
>  };
>  
> +/* Need to free remain_skb allocated in ath9k_hif_usb_rx_stream
> + * in case ath9k_hif_usb_rx_stream wasn't called next time to
> + * process the buffer and subsequently free it.
> + */
> +static void ath9k_hif_usb_free_rx_remain_skb(struct hif_device_usb *hif_dev)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&hif_dev->rx_lock, flags);
> +	if (hif_dev->remain_skb) {
> +		dev_kfree_skb_any(hif_dev->remain_skb);
> +		hif_dev->remain_skb = NULL;
> +		hif_dev->rx_remain_len = 0;
> +	}
> +	spin_unlock_irqrestore(&hif_dev->rx_lock, flags);
> +}
> +
>  static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
>  				    struct sk_buff *skb)
>  {
> @@ -1129,6 +1146,7 @@ static int ath9k_hif_usb_dev_init(struct hif_device_usb *hif_dev)
>  static void ath9k_hif_usb_dev_deinit(struct hif_device_usb *hif_dev)
>  {
>  	ath9k_hif_usb_dealloc_urbs(hif_dev);
> +	ath9k_hif_usb_free_rx_remain_skb(hif_dev);
>  }

Erm, does this actually fix the leak? AFAICT, ath9k_hif_usb_dev_deinit()
is only called on the error path of ath9k_hif_usb_firmware_cb(), not
when the device is subsequently torn down in
ath9k_htc_disconnect_device()?

I think the right place to put this is probably inside
ath9k_hif_usb_dealloc_urbs()? That gets called on USB suspend as well,
but it seems to me that if we're suspending the device to an extent that
we're deallocating the urbs, we should be clearing out the cached skb in
remain_skb anyway?

-Toke