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

[sashiko-bot@kernel.org]

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