usb: gadget: pch_udc: drain vbus GPIO works before freeing the device
Subsystems (2)
PATCH v2 1/1 usb: gadget: pch_udc: drain vbus GPIO works before freeing the device
From: Hyunjung Ko <[email protected]>
pch_vbus_gpio_irq() and pch_udc_dev_isr() schedule dev->vbus_gpio.irq_work_fall / irq_work_rise (pch_vbus_gpio_work_fall / pch_vbus_gpio_work_rise), whose handlers obtain the pch_udc_dev via container_of() and dereference it (dev->driver, dev->pdev, and pch_udc_init()/pch_udc_reconnect()). Nothing cancels these works on teardown.
pch_udc_remove() -> pch_udc_exit() -> pch_vbus_gpio_free() previously only called free_irq() for the VBUS GPIO interrupt; dev is allocated with devm_kzalloc() and is freed by devres after removal. free_irq() only waits for the interrupt handler to finish, it does not cancel a work the handler has already queued, so a pending or running work then runs against the freed pch_udc_dev (use-after-free). pch_vbus_gpio_work_fall() polls for up to PCH_VBUS_PERIOD (3s), widening the window.
The two works are armed differently. pch_vbus_gpio_init() assigns dev->vbus_gpio.port and INIT_WORK()s irq_work_fall *before* request_irq() sets dev->vbus_gpio.intr, and pch_udc_dev_isr() schedules irq_work_fall exactly when dev->vbus_gpio.intr is 0; irq_work_rise is only initialised and scheduled once intr is non-zero. Cancelling both only under "if (dev->vbus_gpio.intr)" therefore misses irq_work_fall whenever the GPIO is present but the IRQ was not acquired (gpiod_to_irq()/request_irq() failure, or a device interrupt racing removal before interrupts are masked).
Drain each work according to how it is armed: cancel irq_work_fall whenever a VBUS GPIO port exists, and cancel irq_work_rise next to free_irq() under the intr guard. free_irq() runs first so the GPIO interrupt can no longer schedule work before it is drained.
Found by static analysis and code review; not runtime-reproduced (needs EG20T/ML7213 pch_udc hardware with a VBUS detection GPIO and an interrupt racing device removal).
---
v2: drain irq_work_fall under the GPIO-port guard rather than the IRQ
guard. irq_work_fall is scheduled from pch_udc_dev_isr() when dev->vbus_gpio.intr == 0, so the v1 "if (intr)" placement skipped it. Spotted by Sashiko AI review. The separate pre-existing missing cancellation in pch_udc_shutdown() that the review also flagged is left for its own patch.
drivers/usb/gadget/udc/pch_udc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +5 -1
@@ -1406,8 +1406,12 @@ static int pch_vbus_gpio_init(struct pch_udc_dev *dev)*/ static void pch_vbus_gpio_free (struct pch_udc_dev *dev) { -if (dev->vbus_gpio.intr) +if (dev->vbus_gpio.intr) {free_irq (dev->vbus_gpio.intr, dev); +cancel_work_sync (&dev->vbus_gpio.irq_work_rise); + } +if (dev->vbus_gpio.port) +cancel_work_sync (&dev->vbus_gpio.irq_work_fall); }/**
Signature · 1 line
2.43.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Medium] usb: gadget: pch_udc: irq_work_fall drained before UDC IRQ is stopped --
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Medium] usb: gadget: pch_udc: irq_work_fall drained before UDC IRQ is stopped
--
commit e559e91d2c5a51fa2b3d43db5cbba0eb72f1363b
Author: Hyunjung Ko <[email protected]>
usb: gadget: pch_udc: drain vbus GPIO works before freeing the device
This commit calls cancel_work_sync() on irq_work_rise and irq_work_fall in pch_vbus_gpio_free() to stop pending VBUS GPIO work items on teardown.
Quoted 8 lines
drivers/usb/gadget/udc/pch_udc.c +0
-0@@ -1406,8 +1406,12 @@ static int pch_vbus_gpio_init(struct pch_udc_dev *dev)*/ static void pch_vbus_gpio_free (struct pch_udc_dev *dev) {
drivers/usb/gadget/udc/pch_udc.c static int pch_vbus_gpio_init(struct pch_udc_dev *dev) · line 1417
Quoted 10 lines
drivers/usb/gadget/udc/pch_udc.c +5
-1-if (dev->vbus_gpio.intr) +if (dev->vbus_gpio.intr) {free_irq (dev->vbus_gpio.intr, dev); +cancel_work_sync (&dev->vbus_gpio.irq_work_rise); + } +if (dev->vbus_gpio.port) +cancel_work_sync (&dev->vbus_gpio.irq_work_fall); }/**
[Severity: Medium]
Can pch_udc_dev_isr() re-arm irq_work_fall after cancel_work_sync() returns?
When dev->vbus_gpio.intr is 0, irq_work_fall is queued from the PCI device interrupt handler pch_udc_isr() -> pch_udc_dev_isr() on UDC_DEVINT_US. Unlike the GPIO interrupt, which free_irq() stops before cancel_work_sync() runs, pch_udc_remove() calls pch_vbus_gpio_free() before pch_udc_exit() masks UDC device interrupts, and pdev->irq is only freed by devres after pch_udc_remove() returns:
CPU0 CPU1pch_udc_remove()
usb_del_gadget_udc()
pch_udc_stop()
pch_udc_isr()
pch_udc_dev_isr()
spin_unlock(&dev->lock)
dev->driver->suspend(...)
pch_vbus_gpio_free()
cancel_work_sync(
&dev->vbus_gpio.irq_work_fall)
spin_lock(&dev->lock)
schedule_work(
&dev->vbus_gpio.irq_work_fall) pch_udc_exit()
/* devres frees dev */
pch_vbus_gpio_work_fall()
vbus = pch_vbus_gpio_get_value(dev)
<-- use-after-free
Even if pch_udc_stop() masked UDC_DEVINT_MSK earlier, it does not take dev->lock or call synchronize_irq(dev->pdev->irq), so an in-flight pch_udc_dev_isr() or a UDC interrupt unmasked while no gadget driver was bound can still schedule irq_work_fall after pch_vbus_gpio_free() finishes.
Also, the commit message describes the teardown call chain as pch_udc_remove() -> pch_udc_exit() -> pch_vbus_gpio_free(), whereas pch_udc_remove() calls pch_vbus_gpio_free() before pch_udc_exit().
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1