Re: [PATCH] net: hso: fix tx skb leak when the device stops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net: hso: race on skb_tx_buf between hso_net_close() and
  hso_resume()
- [Low] net: hso: missing Fixes trailer in commit message
--

commit 759e4b053ce61e8448ad5b49402e8bd576be78fd
Author: Aibing Chen <chenaibing@cmss.chinamobile.com>

> net: hso: fix tx skb leak when the device stops
> 
> hso_net_start_xmit() stashes the skb in odev->skb_tx_buf and stops the
> queue when hso_get_activity() reports -EAGAIN, which happens when the
> USB device is suspended or no longer configured.  If the interface is
> then brought down, or the device is disconnected, that skb is simply
> left behind: hso_net_close() and hso_free_net_device() never release
> it, and hso_net_open() clears the pointer, so the skb and its data
> buffer are leaked.
> 
> Free the pending skb in hso_net_close(), which is also reached from
> unregister_netdev() on disconnect.
> 
> hso_resume() loses a frame in the same way when the device is still not
> ready: it passes skb_tx_buf to hso_net_start_xmit() and then clears the
> pointer unconditionally, so a frame stashed again by that call is never
> freed.  Clear the pointer before the call instead and let
> hso_net_start_xmit() stash it again if it has to.
> 
> Reported-by: syzbot+bf62ca9997f43d2fa521@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/
> Signed-off-by: Aibing Chen <chenaibing@cmss.chinamobile.com>

[Severity: Low]
Should this commit include Fixes tags for the two commits that introduced
these skb_tx_buf leaks?

  Fixes: 72dc1c096c70 ("HSO: add option hso driver")
  Fixes: c213f286f2cf ("[netdrvr] hso: dev_kfree_skb crash fix")

> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 71caa3764b239..03956fa8ee537 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -728,6 +728,13 @@ static int hso_net_close(struct net_device *net)
>  
>  	hso_stop_net_device(odev->parent);
>  
> +	/*
> +	 * A frame stashed by hso_net_start_xmit() will never be transmitted now,
> +	 * so drop it instead of leaking it.
> +	 */
> +	dev_kfree_skb(odev->skb_tx_buf);
> +	odev->skb_tx_buf = NULL;
> +
>  	/* done */
>  	return 0;
>  }
> @@ -3055,12 +3062,13 @@ static int hso_resume(struct usb_interface *iface)
>  				/* First transmit any lingering data,
>  				   then restart the device. */
>  				if (hso_net->skb_tx_buf) {
> +					struct sk_buff *skb = hso_net->skb_tx_buf;
> +
>  					dev_dbg(&iface->dev,
>  						"Transmitting"
>  						" lingering data\n");
> -					hso_net_start_xmit(hso_net->skb_tx_buf,
> -							   hso_net->net);
>  					hso_net->skb_tx_buf = NULL;
> +					hso_net_start_xmit(skb, hso_net->net);
>  				}
>  				result = hso_start_net_device(network_table[i]);
>  				if (result)

[Severity: High]
Can hso_net_close() and hso_resume() race on skb_tx_buf and cause a
use-after-free and double free?

When hso_net_start_xmit() stashes a frame while the USB device is
suspended, hso_get_activity() schedules async_get_intf to resume the
interface.  hso_net_close() runs under rtnl_lock without holding the USB
device lock, cancelling async_get_intf, or synchronizing with hso_resume():

CPU0                              CPU1
hso_resume()                      hso_net_close()
  if (hso_net->flags & IFF_UP)
    skb = hso_net->skb_tx_buf;
                                    clear_bit(HSO_NET_RUNNING, &odev->flags);
                                    hso_stop_net_device(odev->parent);
                                    dev_kfree_skb(odev->skb_tx_buf);
                                    odev->skb_tx_buf = NULL;
    hso_net->skb_tx_buf = NULL;
    hso_net_start_xmit(skb, ...)
      memcpy(..., skb->data, ...) <-- use-after-free
      dev_kfree_skb(skb)          <-- double free

If CPU0 clears hso_net->skb_tx_buf just before CPU1 calls dev_kfree_skb(),
CPU1 frees NULL and finishes closing the device, after which CPU0 calls
hso_net_start_xmit() and hso_start_net_device() on the stopped device,
either submitting URBs after close or re-stashing and leaking skb in
skb_tx_buf.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114093133.465-1-chenaibing@cmss.chinamobile.com?part=1