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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds an unsynchronized free of odev->skb_tx_buf in
  hso_net_close() (drivers/net/usb/hso.c:735-736).
- [Low] The newly added free in hso_net_close()
  (drivers/net/usb/hso.c:731-736) discards a frame that was accepted…
- [Low] The patch fixes a user-visible, syzbot-reported skb leak but
  carries no Fixes: tag (and no Cc: stable).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  hso_net_start_xmit() (drivers/net/usb/hso.c:789) does…

--

net: hso: fix tx skb leak when the device stops

This patch frees the skb that hso_net_start_xmit() stashes in
skb_tx_buf when it runs in hso_net_close(), which is also reached on
disconnect. It also changes hso_resume() so that skb_tx_buf is cleared
before the lingering frame is passed back to hso_net_start_xmit().

Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/

> 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]
This patch fixes a leak that syzbot reported, but it has no Fixes: tag.

The resume path ordering changed here seems to come from commit
89930b7b5e3e ("hso: Fix URB submission -EINVAL."). The stash and
teardown logic dates back to commit 72dc1c096c70 ("HSO: add option hso
driver").

Should one of these be added as a Fixes: tag, along with Cc: stable?

> 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);

[Severity: Low]
The comment says the frame is dropped, but dev_kfree_skb() is defined as
consume_skb(). The kfree_skb drop tracepoint and drop monitor will
never see this frame. net->stats.tx_dropped is not incremented either.

Would kfree_skb() or kfree_skb_reason(), plus net->stats.tx_dropped++,
match the intent better?

> +	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);

[Severity: High]
Can the new free in hso_net_close() race with this replay of the
lingering frame?

hso_net_close() runs as ndo_stop and holds only RTNL. hso_resume() runs
as the USB resume callback. One way it gets there is the async_get_intf
work, which hso_get_activity() schedules at the same moment the frame
is stashed:

hso_get_activity() {
    ...
	if (hso_dev->usb->state == USB_STATE_SUSPENDED) {
		if (!hso_dev->is_active) {
			hso_dev->is_active = 1;
			schedule_work(&hso_dev->async_get_intf);
    ...
}

The two paths share no lock, so this interleaving looks possible:

CPU1 (hso_resume)                    CPU2 (hso_net_close)
if (hso_net->flags & IFF_UP)
if (hso_net->skb_tx_buf)
skb = hso_net->skb_tx_buf;
                                     clear_bit(HSO_NET_RUNNING, ...);
                                     hso_stop_net_device(...);
                                     dev_kfree_skb(odev->skb_tx_buf);
hso_net->skb_tx_buf = NULL;
hso_net_start_xmit(skb, ...)
    memcpy(..., skb->data, skb->len);
    ...
    dev_kfree_skb(skb);

The device is configured again during resume. hso_net_start_xmit()
would therefore read the freed skb data, send it to the device, and then
free the skb a second time.

Before this patch, hso_net_close() never freed skb_tx_buf, so this race
could only cause a late transmit or a leak.

Clearing the pointer earlier in hso_resume() does not seem to close the
window. The load and the clear are still not atomic with respect to
close.

Could the stash, the replay and the free be serialized, for example
with net_lock or netif_tx_lock? HSO_NET_RUNNING could then be rechecked
under that lock. Another option would be
cancel_work_sync(&async_get_intf) in hso_net_close().

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Since
the patch adds another caller of hso_net_start_xmit(), though: can that
function overflow mux_bulk_tx_buf?

hso_net_start_xmit() copies skb->len bytes without checking them
against MUX_BULK_TX_BUF_SIZE:

	memcpy(odev->mux_bulk_tx_buf, skb->data, skb->len);

The buffer has a fixed size, set in hso_create_net_device():

	hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);

hso_net_init() only sets net->mtu = DEFAULT_MTU - 14 and never sets
net->max_mtu. As a result, dev_validate_mtu() skips the upper bound
check:

	if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) {

After something like "ip link set hsoX mtu 9000", any packet larger
than 2048 bytes would overflow the kmalloc-2048 object. skb->len is
also used as the URB transfer length, so the USB controller would read
past the buffer as well.

Should hso_net_init() set net->max_mtu, or should
hso_net_start_xmit() check skb->len before the memcpy()?

>  				}
>  				result = hso_start_net_device(network_table[i]);
>  				if (result)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114093133.465-1-chenaibing%40cmss.chinamobile.com