Re: [PATCH] usb: isp1760: shut down the VBUS timer before unregistering the UDC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: isp1760: vbus_timer rearming in isp1760_udc_stop() causes NULL deref
--

commit fa23f41e78ec14a6b9fcdbfcc740696149d64aa0
Author: Myeonghun Pak <mhun512@gmail.com>

usb: isp1760: shut down the VBUS timer before unregistering the UDC

This commit adds timer_shutdown_sync() calls for vbus_timer in
isp1760_udc_unregister() and the error path of isp1760_udc_register() to
prevent the timer from remaining pending after the UDC is unregistered.

> diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
> index e45a54c4a5609..7b60bd9069bc2 100644
> --- a/drivers/usb/isp1760/isp1760-udc.c
> +++ b/drivers/usb/isp1760/isp1760-udc.c

[ ... ]

> @@ -1593,6 +1594,9 @@ void isp1760_udc_unregister(struct isp1760_device *isp)
>  	if (!udc->isp)
>  		return;
>  
> +	/* The IRQ handler can rearm the timer until free_irq() below. */
> +	timer_shutdown_sync(&udc->vbus_timer);
> +
>  	usb_del_gadget_udc(&udc->gadget);
>  
>  	free_irq(udc->irq, udc);

[Severity: High]
What happens if the same race in isp1760_udc_stop() occurs when a gadget
driver is unbound or disconnected via sysfs without unregistering the UDC?

In drivers/usb/isp1760/isp1760-udc.c, isp1760_udc_stop() remains unchanged:

	timer_delete_sync(&udc->vbus_timer);

	isp1760_reg_write(udc->regs, mode_reg, 0);

	spin_lock_irqsave(&udc->lock, flags);
	udc->driver = NULL;
	spin_unlock_irqrestore(&udc->lock, flags);

When isp1760_udc_stop() is called without isp1760_udc_unregister() (for
example via gadget_unbind_driver() or soft_connect_store()), vbus_timer is
not shut down. If isp1760_udc_irq() handles ISP176x_DC_IEVBUS after
timer_delete_sync() completes, isp1760_udc_connect() runs under udc->lock
without checking udc->driver:

	usb_gadget_set_state(&udc->gadget, USB_STATE_POWERED);
	mod_timer(&udc->vbus_timer, jiffies + ISP1760_VBUS_POLL_INTERVAL);

isp1760_udc_stop() then clears udc->driver = NULL and returns while
vbus_timer is still pending.

When vbus_timer fires, isp1760_udc_vbus_poll() calls
isp1760_udc_disconnect() if DC_VBUSSTAT is not set:

	if (!(isp1760_udc_is_set(udc, DC_VBUSSTAT)))
		isp1760_udc_disconnect(udc);

And isp1760_udc_disconnect() dereferences udc->driver without checking for
NULL because udc->gadget.state was set back to USB_STATE_POWERED:

	if (udc->driver->disconnect)
		udc->driver->disconnect(&udc->gadget);

Could this trigger a NULL pointer dereference of udc->driver in softirq
timer context? Should isp1760_udc_stop() also prevent isp1760_udc_irq()
from rearming vbus_timer (or cancel the timer after disabling interrupts
and clearing udc->driver)?

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