[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