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

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

[Aibing Chen]

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>
---
 drivers/net/usb/hso.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 71caa3764..03956fa8e 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)

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

[sashiko-bot]

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

[PATCH v2] net: hso: fix tx skb leak and a race on the stashed skb

[Aibing Chen]

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.

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.

Free the pending skb in hso_net_close() and take ownership of the
pointer with xchg() in both places, so that only one of the close and
resume paths can obtain it.  hso_resume() also drops the frame instead
of transmitting it when the interface has already been stopped.

Reported-by: syzbot+bf62ca9997f43d2fa521@syzkaller.appspotmail.com
Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/
Fixes: 72dc1c096c70 ("HSO: add option hso driver")
Fixes: c213f286f2cf ("[netdrvr] hso: dev_kfree_skb crash fix")
Signed-off-by: Aibing Chen <chenaibing@cmss.chinamobile.com>
---
v2:
- take the stashed skb with xchg() in hso_net_close() and hso_resume(),
  so that only one path can free it (race reported by sashiko).
- drop the frame in hso_resume() instead of transmitting it after the
  interface was stopped.
- add the two Fixes tags.

 drivers/net/usb/hso.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 71caa3764..ba59bdefd 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -720,6 +720,7 @@ static int hso_net_open(struct net_device *net)
 static int hso_net_close(struct net_device *net)
 {
 	struct hso_net *odev = netdev_priv(net);
+	struct sk_buff *skb;
 
 	/* we don't need the queue anymore */
 	netif_stop_queue(net);
@@ -728,6 +729,14 @@ static int hso_net_close(struct net_device *net)
 
 	hso_stop_net_device(odev->parent);
 
+	/*
+	 * Take ownership of a frame stashed by hso_net_start_xmit() and drop it.
+	 * hso_resume() takes it the same way, so only one of the two paths frees
+	 * it.
+	 */
+	skb = xchg(&odev->skb_tx_buf, NULL);
+	dev_kfree_skb(skb);
+
 	/* done */
 	return 0;
 }
@@ -3032,6 +3041,7 @@ static int hso_resume(struct usb_interface *iface)
 {
 	int i, result = 0;
 	struct hso_net *hso_net;
+	struct sk_buff *skb;
 
 	/* Start all serial ports */
 	for (i = 0; i < HSO_SERIAL_TTY_MINORS; i++) {
@@ -3054,13 +3064,17 @@ static int hso_resume(struct usb_interface *iface)
 			if (hso_net->flags & IFF_UP) {
 				/* First transmit any lingering data,
 				   then restart the device. */
-				if (hso_net->skb_tx_buf) {
+				skb = xchg(&hso_net->skb_tx_buf, NULL);
+				if (skb) {
 					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;
+					if (test_bit(HSO_NET_RUNNING,
+						     &hso_net->flags))
+						hso_net_start_xmit(skb,
+								   hso_net->net);
+					else
+						dev_kfree_skb(skb);
 				}
 				result = hso_start_net_device(network_table[i]);
 				if (result)