Re: [PATCH 17/19] usb: renesas_usbhs: unlink the URB if it is not queued

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: renesas_usbhs: new_udev leak in usbhsh_urb_enqueue() on queue push error
--

commit 3025db27a621582451ab2718c744f405f863e1c5
Author: Josef Bacik <josef@toxicpanda.com>

usb: renesas_usbhs: unlink the URB if it is not queued

This commit fixes usbhsh_urb_enqueue() to unlink the URB from its endpoint
and detach the pipe when queueing fails, preventing list corruption and pipe
use count leaks on enqueue errors.

> diff --git a/drivers/usb/renesas_usbhs/mod_host.c b/drivers/usb/renesas_usbhs/mod_host.c
> index 0652525d06fec..bdec4eaf34027 100644
> --- a/drivers/usb/renesas_usbhs/mod_host.c
> +++ b/drivers/usb/renesas_usbhs/mod_host.c
> @@ -987,7 +987,7 @@ static int usbhsh_urb_enqueue(struct usb_hcd *hcd,
>  		if (!new_udev) {
>  			ret = -EIO;
>  			dev_err(dev, "device attach failed\n");
> -			goto usbhsh_urb_enqueue_error_not_linked;
> +			goto usbhsh_urb_enqueue_error_unlink;
>  		}
>  	}
>  
> @@ -1020,14 +1020,22 @@ static int usbhsh_urb_enqueue(struct usb_hcd *hcd,
>  		ret = usbhsh_dcp_queue_push(hcd, urb, mem_flags);
>  	else
>  		ret = usbhsh_queue_push(hcd, urb, mem_flags);
> +	if (ret < 0) {
> +		usbhsh_pipe_detach(hpriv, usbhsh_ep_to_uep(ep));
> +		goto usbhsh_urb_enqueue_error_unlink;
> +	}
>  
> -	return ret;
> +	return 0;
>  
>  usbhsh_urb_enqueue_error_free_endpoint:
>  	usbhsh_endpoint_detach(hpriv, ep);
>  usbhsh_urb_enqueue_error_free_device:
>  	if (new_udev)
>  		usbhsh_device_detach(hpriv, new_udev);
> +usbhsh_urb_enqueue_error_unlink:

[Severity: High]
Can jumping directly to usbhsh_urb_enqueue_error_unlink when
usbhsh_dcp_queue_push() or usbhsh_queue_push() fails leak the newly attached
new_udev slot in hpriv->udev[] and leave a dangling pointer in
new_udev->usbv?

When the first URB is submitted to a newly connected device at address 0,
usbhsh_urb_enqueue() allocates new_udev via usbhsh_device_attach(), which
claims a slot in hpriv->udev[] and sets new_udev->usbv = usbv.

Next, usbhsh_endpoint_attach() attaches ep0's struct usbhsh_ep to the device
returned by usbhsh_device_get():

drivers/usb/renesas_usbhs/mod_host.c:usbhsh_device_get() {
	...
	/* if it is device0, return it */
	if (0 == usb_pipedevice(urb->pipe))
		return usbhsh_device0(hpriv);
	...
}

Because the device is still at address 0, ep0's endpoint is linked onto
udev0 rather than new_udev.

If usbhsh_dcp_queue_push() or usbhsh_queue_push() then fails, jumping to
usbhsh_urb_enqueue_error_unlink skips usbhsh_urb_enqueue_error_free_endpoint
and usbhsh_urb_enqueue_error_free_device. When enumeration later aborts and
usbhsh_endpoint_disable() runs for ep0, it only sees udev0 and calls
usbhsh_device_detach(hpriv, udev0), which returns early without detaching
new_udev:

drivers/usb/renesas_usbhs/mod_host.c:usbhsh_device_detach() {
	...
	if (0 == usbhsh_device_number(hpriv, udev))
		return;
	...
}

Should this error path jump to usbhsh_urb_enqueue_error_free_endpoint (which
also calls usbhsh_pipe_detach() if a pipe is attached) or otherwise ensure
new_udev is detached?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-llm-usb-v1-0-b9751ddb9c7c@linuxfoundation.org?part=17