netdev - Re: [net,2/3] can: gs_usb: gs_usb_receive_bulk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20260110223836.3890248-1-kuba@kernel.org>
Date: Sat, 10 Jan 2026 14:38:36 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: mkl@...gutronix.de
Cc: Jakub Kicinski <kuba@...nel.org>,
	netdev@...r.kernel.org,
	mailhol@...nel.org,
	ondrej.ille@...il.com,
	linux-can@...r.kernel.org,
	davem@...emloft.net,
	kernel@...gutronix.de,
	swilczek.lx@...il.com,
	pisa@....cvut.cz,
	syzbot+e8cb6691a7cf68256cb8@...kaller.appspotmail.com,
	stable@...r.kernel.org
Subject: Re: [net,2/3] can: gs_usb: gs_usb_receive_bulk_callback(): fix URB memory leak

This is an AI-generated review of your patch.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
can: gs_usb: gs_usb_receive_bulk_callback(): fix URB memory leak

This patch fixes a memory leak by re-anchoring the URB in
gs_usb_receive_bulk_callback() before resubmission. The USB framework
unanchors URBs before calling the completion callback, so the URB needs to
be re-anchored to be tracked by usb_kill_anchored_urbs() in gs_can_close().

Link: https://patch.msgid.link/20260105-gs_usb-fix-memory-leak-v2-1-cc6ed6438034@pengutronix.de

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index a0233e550a5a..d093babbc320 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -751,6 +751,8 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  			  hf, parent->hf_size_rx,
>  			  gs_usb_receive_bulk_callback, parent);
>
> +	usb_anchor_urb(urb, &parent->rx_submitted);
> +
>  	rc = usb_submit_urb(urb, GFP_ATOMIC);

If usb_submit_urb() fails here, the URB remains anchored but is never
submitted. When gs_can_close() later calls usb_kill_anchored_urbs(), it
iterates over anchored URBs and calls usb_kill_urb() on each one. For URBs
that were never successfully submitted, use_count is 0, so usb_kill_urb()
returns without unanchoring them. This causes usb_kill_anchored_urbs() to
loop infinitely on the same URB since the anchor list never becomes empty.

Other CAN USB drivers handle this by calling usb_unanchor_urb() when
submission fails. For example, peak_usb_read_bulk_callback() does:

    usb_anchor_urb(urb, &dev->rx_submitted);
    err = usb_submit_urb(urb, GFP_ATOMIC);
    if (!err)
        return;

    usb_unanchor_urb(urb);

Should similar error handling be added here to avoid potential hangs when
closing the device after a URB submission failure?

>
>  	/* USB failure take down all interfaces */
>  	if (rc == -ENODEV) {

[ ... ]