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