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

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

[Myeonghun Pak]

The VBUS interrupt handler can arm vbus_timer until its IRQ is freed.
Although isp1760_udc_stop() deletes the timer synchronously, it does so
before disabling the controller. An interrupt in that window can rearm
the timer, leaving it pending after usb_del_gadget_udc() and free_irq().
The timer then accesses the devm-allocated controller state after
removal.

Shut down the timer before unregistering the gadget so that pending
callbacks are drained while the gadget is still registered and further
IRQ-driven rearming is ignored. Also shut it down on registration
failure after releasing the IRQ. Keep the temporary stop operation
unchanged so that a gadget driver can subsequently be bound again.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: 0316ca6319b9 ("usb: isp1760: Add device controller support")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---
Compile-tested with Clang, W=1. No hardware runtime testing.

Based on mainline 0fcb05b72344f40806cbe810de8872f1698bf053.

diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
index e45a54c..7b60bd9 100644
--- a/drivers/usb/isp1760/isp1760-udc.c
+++ b/drivers/usb/isp1760/isp1760-udc.c
@@ -1581,6 +1581,7 @@ int isp1760_udc_register(struct isp1760_device *isp, int irq,
 error:
 	if (udc->irq >= 0)
 		free_irq(udc->irq, udc);
+	timer_shutdown_sync(&udc->vbus_timer);
 	kfree(udc->irqname);
 
 	return ret;
@@ -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);

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

[sashiko-bot]

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