[PATCH v2] usb: gadget: pch_udc: drain vbus GPIO works before freeing the device

[PATCH v2] usb: gadget: pch_udc: drain vbus GPIO works before freeing the device

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

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).

Fixes: 637b78eb31e0 ("usb: gadget: pch_udc: Detecting VBUS through GPIO with interrupt")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
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(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index 99b3ce2..5351bd9 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -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);
 }
 
 /**
-- 
2.43.0

Re: [PATCH v2] usb: gadget: pch_udc: drain vbus GPIO works before freeing the device

[sashiko-bot]

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 <hj351016@gmail.com>

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.

> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index 99b3ce28210f9..5351bd9316376 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -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);
>  }
>  
>  /**

[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                              CPU1
pch_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().

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