can: etas_es58x: fix memory leak in es58x_open()

[PATCH] can: etas_es58x: fix memory leak in es58x_open()

Posted by Szymon Wilczek 1 month, 2 weeks ago

When es58x_alloc_rx_urbs() fails partway through allocating RX URBs
(e.g., if usb_submit_urb() fails at iteration i), the function returns
an error but leaves any previously allocated and anchored URBs leaked.

The issue is that es58x_open() returns directly on es58x_alloc_rx_urbs()
failure instead of jumping to the free_urbs label, which would call
es58x_free_urbs() to clean up any anchored URBs.

Fix this by changing 'return ret' to 'goto free_urbs' to ensure proper
cleanup of all allocated URBs on failure.

Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Reported-by: syzbot+e8cb6691a7cf68256cb8@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e8cb6691a7cf68256cb8
Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index f799233c2b72..d92c37f277d5 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -1784,7 +1784,7 @@ static int es58x_open(struct net_device *netdev)
 	if (!es58x_dev->opened_channel_cnt) {
 		ret = es58x_alloc_rx_urbs(es58x_dev);
 		if (ret)
-			return ret;
+			goto free_urbs;
 
 		ret = es58x_set_realtime_diff_ns(es58x_dev);
 		if (ret)
-- 
2.52.0

Re: [PATCH] can: etas_es58x: fix memory leak in es58x_open()

Posted by Vincent Mailhol 1 month, 2 weeks ago

Hi Szymon,

Thanks for the patch.

On 22/12/2025 at 16:42, Szymon Wilczek wrote:
> When es58x_alloc_rx_urbs() fails partway through allocating RX URBs
> (e.g., if usb_submit_urb() fails at iteration i), the function returns
> an error but leaves any previously allocated and anchored URBs leaked.
> 
> The issue is that es58x_open() returns directly on es58x_alloc_rx_urbs()
> failure instead of jumping to the free_urbs label, which would call
> es58x_free_urbs() to clean up any anchored URBs.

The intended logic is that if es58x_alloc_rx_urbs() only partially
succeeds (i.e. submits at least one URB), then you are still able
to continue.

To keep the intended logic, the final:

  return ret;

of es58x_alloc_rx_urbs() should be changed to:

  return 0;

> Fix this by changing 'return ret' to 'goto free_urbs' to ensure proper
> cleanup of all allocated URBs on failure.

If you want to treat a partial allocation of the URBs as a
non-recoverable error, then fine, but this fix is not elegant.

In that case, please do the clean-up in es58x_alloc_rx_urbs() and
update the error message accordingly.

> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
> Reported-by: syzbot+e8cb6691a7cf68256cb8@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e8cb6691a7cf68256cb8
> Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>
> ---
>  drivers/net/can/usb/etas_es58x/es58x_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index f799233c2b72..d92c37f277d5 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -1784,7 +1784,7 @@ static int es58x_open(struct net_device *netdev)
>  	if (!es58x_dev->opened_channel_cnt) {
>  		ret = es58x_alloc_rx_urbs(es58x_dev);
>  		if (ret)
> -			return ret;
> +			goto free_urbs;
>  
>  		ret = es58x_set_realtime_diff_ns(es58x_dev);
>  		if (ret)

Yours sincerely,
Vincent Mailhol

[PATCH v2] can: etas_es58x: allow partial RX URB allocation to succeed

Posted by Szymon Wilczek 1 month, 2 weeks ago

When es58x_alloc_rx_urbs() fails to allocate the requested number of
URBs but succeeds in allocating some, it returns an error code.
This causes es58x_open() to return early, skipping the cleanup label
'free_urbs', which leads to the anchored URBs being leaked.

As pointed out by maintainer Vincent Mailhol, the driver is designed
to handle partial URB allocation gracefully. Therefore, partial
allocation should not be treated as a fatal error.

Modify es58x_alloc_rx_urbs() to return 0 if at least one URB has been
allocated, restoring the intended behavior and preventing the leak
in es58x_open().

Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Reported-by: syzbot+e8cb6691a7cf68256cb8@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e8cb6691a7cf68256cb8
Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>
---
Changes in v2:
- Replaced the cleanup fix in es58x_open() with the logic fix in
  es58x_alloc_rx_urbs() to treat partial allocation as success, as
  suggested by Vincent Mailhol.
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index f799233c2b72..2d248deb69dc 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -1736,7 +1736,7 @@ static int es58x_alloc_rx_urbs(struct es58x_device *es58x_dev)
 	dev_dbg(dev, "%s: Allocated %d rx URBs each of size %u\n",
 		__func__, i, rx_buf_len);
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.52.0

Re: [PATCH v2] can: etas_es58x: allow partial RX URB allocation to succeed

Posted by Marc Kleine-Budde 1 month, 2 weeks ago

On 23.12.2025 02:17:32, Szymon Wilczek wrote:
> When es58x_alloc_rx_urbs() fails to allocate the requested number of
> URBs but succeeds in allocating some, it returns an error code.
> This causes es58x_open() to return early, skipping the cleanup label
> 'free_urbs', which leads to the anchored URBs being leaked.
>
> As pointed out by maintainer Vincent Mailhol, the driver is designed
> to handle partial URB allocation gracefully. Therefore, partial
> allocation should not be treated as a fatal error.
>
> Modify es58x_alloc_rx_urbs() to return 0 if at least one URB has been
> allocated, restoring the intended behavior and preventing the leak
> in es58x_open().
>
> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
> Reported-by: syzbot+e8cb6691a7cf68256cb8@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e8cb6691a7cf68256cb8
> Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>

Applied to linux-can.

Thanks,
Marc

P.S.: There's no need to send a vN+1 patch as reply to vN

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: [PATCH v2] can: etas_es58x: allow partial RX URB allocation to succeed

Posted by Vincent Mailhol 1 month, 2 weeks ago

On 23/12/2025 at 02:17, Szymon Wilczek wrote:
> When es58x_alloc_rx_urbs() fails to allocate the requested number of
> URBs but succeeds in allocating some, it returns an error code.
> This causes es58x_open() to return early, skipping the cleanup label
> 'free_urbs', which leads to the anchored URBs being leaked.
> 
> As pointed out by maintainer Vincent Mailhol, the driver is designed
> to handle partial URB allocation gracefully. Therefore, partial
> allocation should not be treated as a fatal error.
> 
> Modify es58x_alloc_rx_urbs() to return 0 if at least one URB has been
> allocated, restoring the intended behavior and preventing the leak
> in es58x_open().
> 
> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
> Reported-by: syzbot+e8cb6691a7cf68256cb8@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e8cb6691a7cf68256cb8
> Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>

Thanks for the fix!

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>


Yours sincerely,
Vincent Mailhol