Re: [PATCH v3] can: usb: etas_es58x: Fix RX buffer leak on URB resubmit failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The TX URB is accessed after it is freed when es58x_submit_urb() fails, leading to a double free and Use-After-Free.
- [High] usb_free_coherent() is called from an URB completion handler (atomic context), violating IRQ context safety rules.
--

commit fe545bd17f0a8dbd30c2399351f0a4beb7cf3f76
Author: Guangshuo Li <lgs201920130244@gmail.com>

can: usb: etas_es58x: Fix RX buffer leak on URB resubmit failure

es58x_read_bulk_callback() resubmits the RX URB after processing a
received packet. If the resubmit succeeds, the URB remains anchored and
will be handled by the normal RX path or by teardown.

> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index b259f61098083..e1724ae79c5a1 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -1476,7 +1476,6 @@ static void es58x_read_bulk_callback(struct urb *urb)
>  		dev_err_ratelimited(dev,
>  				    "Failed resubmitting read bulk urb: %pe\n",
>  				    ERR_PTR(ret));
> -	return;
>  
>   free_urb:
>  	usb_free_coherent(urb->dev, urb->transfer_buffer_length,

[Severity: High]
This is a pre-existing issue, but does removing this return statement expand a 
bug where usb_free_coherent() is called from an URB completion handler?

es58x_read_bulk_callback() executes in atomic context. usb_free_coherent()
wraps dma_free_coherent() which may sleep on some architectures when unmapping
memory. 

Can this cause a "BUG: scheduling while atomic" panic during RX resubmit
failure?

[Severity: High]
This isn't a bug introduced by this patch, but does the transmit error path have 
a double free when es58x_submit_urb() fails?

If usb_submit_urb() fails during packet transmission:

drivers/net/can/usb/etas_es58x/es58x_core.c:es58x_submit_urb() {
	...
	usb_unanchor_urb(urb);
	...
	usb_free_urb(urb);
}

The pointer is only set to NULL on success, leaving it dangling on failure:

drivers/net/can/usb/etas_es58x/es58x_core.c:es58x_xmit_commit() {
	...
	ret = es58x_submit_urb(priv->es58x_dev, priv->tx_urb, netdev);
	if (ret == 0)
		priv->tx_urb = NULL;
}

Then es58x_start_xmit() jumps to xmit_failure and calls 
es58x_flush_pending_tx_msg(), which uses the dangling pointer:

drivers/net/can/usb/etas_es58x/es58x_core.c:es58x_flush_pending_tx_msg() {
	...
	usb_anchor_urb(priv->tx_urb, &priv->es58x_dev->tx_urbs_idle);
	...
	usb_free_urb(priv->tx_urb);
}

Can this lead to a use-after-free and memory corruption?

>  			  urb->transfer_buffer, urb->transfer_dma);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706014601.415445-1-lgs201920130244@gmail.com?part=1