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

[PATCH v3] 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. Nothing cancels these works on
teardown, and dev is allocated with devm_kzalloc(), so a work still
pending or running when devres frees dev dereferences freed memory
(use-after-free); pch_vbus_gpio_work_fall() polls for up to
PCH_VBUS_PERIOD (3s), widening the window.

The two works are armed from different sources. irq_work_rise is only
set up and scheduled once dev->vbus_gpio.intr is non-zero, from the GPIO
interrupt handler. irq_work_fall is INIT_WORK()ed whenever a VBUS GPIO is
present, and is scheduled either from the GPIO interrupt or, when
dev->vbus_gpio.intr is 0, from the UDC device interrupt
pch_udc_isr() -> pch_udc_dev_isr() on UDC_DEVINT_US.

pch_udc_remove() calls pch_vbus_gpio_free() before pch_udc_exit() masks
the UDC device interrupts, and the PCI interrupt (pch_udc_isr()) is only
released by devres after pch_udc_remove() returns. So draining the works
in pch_vbus_gpio_free() alone races the UDC interrupt: pch_udc_dev_isr()
can re-queue irq_work_fall after cancel_work_sync() has returned.

Mask the UDC device interrupts and synchronize_irq() the PCI interrupt
before draining, so neither source can schedule the works any more, then
cancel them: irq_work_fall whenever a VBUS GPIO port exists, and
irq_work_rise together with free_irq() under the intr guard. free_irq()
runs before its cancel so the GPIO interrupt cannot re-queue either.

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>
---
v3: the UDC device interrupt (pch_udc_dev_isr(), UDC_DEVINT_US) can re-queue
    irq_work_fall after cancel_work_sync() when no VBUS GPIO IRQ is in use, so
    v2's drain alone still raced it. Mask the UDC device interrupts and
    synchronize_irq() the PCI interrupt in pch_udc_remove() before draining.
    Also corrected the teardown call-chain in the commit log
    (pch_vbus_gpio_free() runs before pch_udc_exit()). Both per Sashiko review.
v2: drain irq_work_fall under the VBUS-GPIO-port guard rather than the IRQ
    guard, since it is scheduled when vbus_gpio.intr == 0. Per Sashiko review.

 drivers/usb/gadget/udc/pch_udc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index 99b3ce2..a22bf91 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);
 }
 
 /**
@@ -3010,9 +3014,17 @@ static void pch_udc_remove(struct pci_dev *pdev)
 		dma_unmap_single(&dev->pdev->dev, dev->dma_addr,
 				 UDC_EP0OUT_BUFF_SIZE * 4, DMA_FROM_DEVICE);
 
-	pch_vbus_gpio_free(dev);
-
+	/*
+	 * irq_work_fall can be queued from pch_udc_dev_isr() (UDC_DEVINT_US)
+	 * when no VBUS GPIO interrupt is in use, and the PCI interrupt is only
+	 * released by devres after this function returns. Mask the UDC device
+	 * interrupts and wait for any in-flight handler before draining the
+	 * VBUS GPIO works, so the interrupt can no longer re-queue them.
+	 */
 	pch_udc_exit(dev);
+	synchronize_irq(dev->pdev->irq);
+
+	pch_vbus_gpio_free(dev);
 }
 
 static int __maybe_unused pch_udc_suspend(struct device *d)
-- 
2.43.0

Re: [PATCH v3] 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: draining VBUS works after
  pch_udc_exit() reconnects UDC
--

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

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

This commit masks UDC device interrupts, synchronizes the PCI interrupt,
and cancels the VBUS GPIO work items during device removal before devres
frees the device structure.

> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index 99b3ce28210f9..a22bf91e0bfc4 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);
>  }
>  
>  /**
> @@ -3010,9 +3014,17 @@ static void pch_udc_remove(struct pci_dev *pdev)
>  		dma_unmap_single(&dev->pdev->dev, dev->dma_addr,
>  				 UDC_EP0OUT_BUFF_SIZE * 4, DMA_FROM_DEVICE);
>  
> -	pch_vbus_gpio_free(dev);
> -
> +	/*
> +	 * irq_work_fall can be queued from pch_udc_dev_isr() (UDC_DEVINT_US)
> +	 * when no VBUS GPIO interrupt is in use, and the PCI interrupt is only
> +	 * released by devres after this function returns. Mask the UDC device
> +	 * interrupts and wait for any in-flight handler before draining the
> +	 * VBUS GPIO works, so the interrupt can no longer re-queue them.
> +	 */
>  	pch_udc_exit(dev);
> +	synchronize_irq(dev->pdev->irq);
> +
> +	pch_vbus_gpio_free(dev);
>  }

[Severity: Medium]
Can an in-flight VBUS work undo pch_udc_exit(dev) while
pch_vbus_gpio_free() waits for it?

Both pch_vbus_gpio_work_rise() and pch_vbus_gpio_work_fall() call
pch_udc_reconnect(dev), which soft-resets the controller, clears
UDC_DEVCTL_SD, and unmasks UDC_DEVINT_UR | UDC_DEVINT_ENUM:

drivers/usb/gadget/udc/pch_udc.c:pch_udc_reconnect() {
	pch_udc_init(dev);

	/* enable device interrupts */
	/* pch_udc_enable_interrupts() */
	pch_udc_bit_clr(dev, UDC_DEVIRQMSK_ADDR,
			UDC_DEVINT_UR | UDC_DEVINT_ENUM);

	/* Clear the disconnect */
	pch_udc_bit_set(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_RES);
	pch_udc_bit_clr(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_SD);
...
}

If either work is already running (or queued by pch_vbus_gpio_irq() before
free_irq()) when pch_udc_remove() moves pch_udc_exit() ahead of
pch_vbus_gpio_free(), the controller is re-enabled and left connected while
dev->pdev->irq is still active:

CPU0                                  kworker
pch_udc_remove()
  dma_pool_destroy(dev->stp_requests)
  pch_udc_exit(dev)
  synchronize_irq(dev->pdev->irq)
  pch_vbus_gpio_free(dev)
    cancel_work_sync(...)
                                      pch_vbus_gpio_work_rise()
                                        pch_udc_reconnect(dev)
  /* UDC left connected with UDC_DEVINT_ENUM unmasked */
  <hardirq>
  pch_udc_isr()
    pch_udc_dev_isr()
      pch_udc_svc_enum_interrupt()
        pch_udc_activate_control_ep()
          *dev->ep[...].td_stp = ...  <-- use-after-free

pch_udc_svc_enum_interrupt() also unmasks UDC_DEVINT_US again, allowing
pch_udc_dev_isr() to re-queue irq_work_fall after cancel_work_sync() has
returned.  Calling pch_udc_exit(dev) and synchronize_irq(dev->pdev->irq)
before dma_pool_destroy() and again after pch_vbus_gpio_free(dev) would
ensure the hardware stays masked and disconnected.

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