Re: Forwarded: [PATCH] usb: misc: uss720: fix memory leak by replacing usb_unlink_urb with usb_kill_urb

[David Laight]

On Thu, 16 Jul 2026 05:09:22 -0700
syzbot <syzbot+56962eb32ba0136cd330@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

> For archival purposes, forwarding an incoming command email to
> linux-kernel@xxxxxxxxxxxxxxx, syzkaller-bugs@xxxxxxxxxxxxxxxx.
> 
> ***
> 
> Subject: [PATCH] usb: misc: uss720: fix memory leak by replacing usb_unlink_urb with usb_kill_urb
> Author: kartikey406@xxxxxxxxx
> 
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> 
> 
> kmemleak reported memory leaks in uss720_probe() during device
> initialization. The leaks occur because kill_all_async_requests_priv()
> was using usb_unlink_urb() which is asynchronous and returns immediately
> without waiting for the URB completion callbacks to finish.
> 
> This caused a race condition where kref_put() in the probe_abort path
> would run before async_complete() had a chance to call destroy_async(),
> leaving the URB, request struct (uss720_async_request), and transfer
> buffer (dr) unreferenced and leaked.
> 
> Fix this by replacing usb_unlink_urb() with usb_kill_urb() which blocks
> until the completion callback has finished, ensuring destroy_async()
> runs and frees all associated memory before kref_put() is called on
> priv.
> 
> Since usb_kill_urb() may sleep, it cannot be called while holding a
> spinlock. Drop and reacquire the spinlock around each usb_kill_urb()
> call. Also switch to list_for_each_entry_safe() to safely handle list
> modification during iteration, as destroy_async() calls list_del_init()
> on the current entry.
> 
> Reported-by: syzbot+56962eb32ba0136cd330@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: http://syzkaller.appspot.com/bug?extid=56962eb32ba0136cd330
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> ---
>  drivers/usb/misc/uss720.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c
> index 1ce48f5832d7..f43fbb28044b 100644
> --- a/drivers/usb/misc/uss720.c
> +++ b/drivers/usb/misc/uss720.c
> @@ -175,13 +175,15 @@ static struct uss720_async_request *submit_async_request(struct parport_uss720_p
>  
>  static unsigned int kill_all_async_requests_priv(struct parport_uss720_private *priv)
>  {
> -	struct uss720_async_request *rq;
> +	struct uss720_async_request *rq, *tmp;
>  	unsigned long flags;
>  	unsigned int ret = 0;
>  
>  	spin_lock_irqsave(&priv->asynclock, flags);
> -	list_for_each_entry(rq, &priv->asynclist, asynclist) {
> -		usb_unlink_urb(rq->urb);
> +	list_for_each_entry_safe(rq, tmp, &priv->asynclist, asynclist) {
> +		spin_unlock_irqrestore(&priv->asynclock, flags);
> +		usb_kill_urb(rq->urb);
> +		spin_lock_irqsave(&priv->asynclock, flags);

I'll take a bet on that not being correct.
The _safe() only lets you remove the current item.
So if the list can be changed by someone else it doesn't help.
Possibly this is tidy up code and the lock just isn't needed?
Maybe the head of the list should be removed under the lock?
(and the rest of the tidy up done at the same time.)

	David

>  		ret++;
>  	}
>  	spin_unlock_irqrestore(&priv->asynclock, flags);