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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

pch_vbus_gpio_irq() schedules dev->vbus_gpio.irq_work_fall /
irq_work_rise (pch_vbus_gpio_work_fall/_rise), whose handlers obtain the
pch_udc_dev via container_of() and dereference it.

pch_vbus_gpio_free() frees the VBUS GPIO interrupt on teardown but never
cancels those works:

	if (dev->vbus_gpio.intr)
		free_irq(dev->vbus_gpio.intr, dev);

free_irq() only waits for the interrupt handler to finish; it does not
cancel a work the handler has already queued. dev is allocated with
devm_kzalloc() and is freed by devres after pch_udc_remove() returns, so
a VBUS GPIO interrupt arriving shortly before removal can leave
irq_work_fall/rise pending and have pch_vbus_gpio_work_fall()/_rise() run
against the freed pch_udc_dev -> use-after-free.

Cancel both works after free_irq() (so the interrupt can no longer
re-queue them) and before dev is freed. Both are INIT_WORK()'d on the
path that sets vbus_gpio.intr, so the existing intr guard also covers the
cancels.

Found by code review; not reproduced (needs the 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>
---
 drivers/usb/gadget/udc/pch_udc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

Re: [PATCH] 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: 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