Re: [PATCH] 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: uncancelled irq_work_fall in
  pch_vbus_gpio_free()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] usb: gadget: pch_udc: missing work cancellation in
  pch_udc_shutdown() [1]
--

commit 89d87fae917e9ebb35853723c8f26eea53836dd5
Author: Hyunjung Ko <hj351016@gmail.com>

usb: gadget: pch_udc: drain vbus GPIO works before freeing the device

This commit cancels dev->vbus_gpio.irq_work_fall and irq_work_rise after
free_irq() in pch_vbus_gpio_free() to prevent the VBUS GPIO work handlers
from running after dev is freed on device removal.

> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index 99b3ce28210f9..55310946de292 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -1406,8 +1406,11 @@ 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_fall);
> +		cancel_work_sync(&dev->vbus_gpio.irq_work_rise);
> +	}
>  }
>  
>  /**

[Severity: Medium]
Can dev->vbus_gpio.irq_work_fall still be left running when
dev->vbus_gpio.port is present but dev->vbus_gpio.intr is 0?

pch_vbus_gpio_init() initializes irq_work_fall whenever a VBUS GPIO is
obtained, before gpiod_to_irq() and request_irq() set dev->vbus_gpio.intr:

drivers/usb/gadget/udc/pch_udc.c:pch_vbus_gpio_init() {
	...
	dev->vbus_gpio.port = gpiod;
	INIT_WORK(&dev->vbus_gpio.irq_work_fall, pch_vbus_gpio_work_fall);

	irq_num = gpiod_to_irq(gpiod);
	...
}

When dev->vbus_gpio.intr is 0, pch_udc_dev_isr() schedules irq_work_fall
on a USB suspend interrupt:

drivers/usb/gadget/udc/pch_udc.c:pch_udc_dev_isr() {
	...
		} else if ((dev->vbus_session == 0)
			&& (vbus == 1)
			&& !dev->vbus_gpio.intr)
			schedule_work(&dev->vbus_gpio.irq_work_fall);
	...
}

If gpiod_to_irq() or request_irq() fails, or if UDC_DEVINT_US fires in
pch_udc_remove() before pch_udc_exit() masks device interrupts,
pch_vbus_gpio_work_fall() can continue polling for up to 3 seconds after
pch_udc_remove() returns and devres frees dev, causing a use-after-free.

[Severity: High]
This problem wasn't introduced by this patch, but can a VBUS GPIO
transition during system shutdown re-initialize the controller after
pch_udc_shutdown() [1]?

pch_udc_shutdown() disables UDC interrupts and disconnects the pullup,
but it does not call pch_vbus_gpio_free() or otherwise disable the VBUS
GPIO interrupt and cancel irq_work_fall and irq_work_rise. If either work
runs during shutdown, pch_vbus_gpio_work_fall() or
pch_vbus_gpio_work_rise() can call pch_udc_init() or pch_udc_reconnect(),
resetting the hardware and re-enabling PCI interrupts across reboot or
kexec.

[1] https://sashiko.dev/#/bug/linux-bb1818ac-db9a-4dc2-af47-62b314381f33

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