[PATCH] usb: gadget: lpc32xx_udc: drain the pullup work on removal

[PATCH] usb: gadget: lpc32xx_udc: drain the pullup work on removal

[Hyunjung Ko]

isp1301_pullup_enable() schedules udc->pullup_job to perform the slow
isp1301 i2c pull-up access from process context. The work handler
(pullup_work) dereferences udc and touches the isp1301 i2c client.

udc is allocated with devm_kmemdup(), so devres frees it after
lpc32xx_udc_remove() returns. remove() calls pullup(udc, 0) -- which can
schedule pullup_job -- and also puts the isp1301 i2c client, but it never
drains the work. A pullup_job still pending can then run after udc has
been freed (and the isp1301 client put), dereferencing freed memory -- a
use-after-free.

Cancel pullup_job in lpc32xx_udc_remove(), after the final pullup(udc, 0)
and before the udc and its isp1301 client are released.

Found by static analysis; not runtime-reproduced (needs LPC32xx USB
gadget hardware with the isp1301 transceiver).

Fixes: 24a28e428351 ("USB: gadget driver for LPC32xx")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/usb/gadget/udc/lpc32xx_udc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
index 94355d3..210fa1f 100644
--- a/drivers/usb/gadget/udc/lpc32xx_udc.c
+++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
@@ -3186,6 +3186,13 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
 	udc_clk_set(udc, 1);
 	udc_disable(udc);
 	pullup(udc, 0);

+	/*
+	 * pullup() schedules pullup_job to do the slow isp1301 i2c access; the
+	 * work dereferences udc, so drain it before the devm-allocated udc (and
+	 * the isp1301 i2c client it uses) are released.
+	 */
+	cancel_work_sync(&udc->pullup_job);
+
 	device_init_wakeup(&pdev->dev, 0);
 	remove_debug_file(udc);

--
2.43.0

Re: [PATCH] usb: gadget: lpc32xx_udc: drain the pullup work on removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: lpc32xx_udc: cancel_work_sync() skips
  pullup_work() on remove
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] usb: gadget: lpc32xx_udc: incorrect work struct scheduled in
  udc_handle_dev() [1]
--

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

usb: gadget: lpc32xx_udc: drain the pullup work on removal

This commit cancels udc->pullup_job in lpc32xx_udc_remove() before releasing
the devm-allocated udc and its isp1301 i2c client.

> diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
> index 94355d3d930fd..210fa1f886c8a 100644
> --- a/drivers/usb/gadget/udc/lpc32xx_udc.c
> +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
> @@ -3187,6 +3187,13 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
>  	udc_disable(udc);
>  	pullup(udc, 0);
>  
> +	/*
> +	 * pullup() schedules pullup_job to do the slow isp1301 i2c access; the
> +	 * work dereferences udc, so drain it before the devm-allocated udc (and
> +	 * the isp1301 i2c client it uses) are released.
> +	 */
> +	cancel_work_sync(&udc->pullup_job);

[Severity: Medium]
Does cancel_work_sync(&udc->pullup_job) skip disabling the ISP1301 D+
pull-up on removal?

pullup(udc, 0) calls isp1301_pullup_enable(udc, 0, 0) with block = 0, which
sets udc->pullup = 0 and queues udc->pullup_job so pullup_work() ->
isp1301_pullup_set() clears OTG1_DP_PULLUP over I2C:

drivers/usb/gadget/udc/lpc32xx_udc.c:isp1301_pullup_enable() {
	if (en_pullup == udc->pullup)
		return;

	udc->pullup = en_pullup;
	if (block)
		isp1301_pullup_set(udc);
	else
		/* defer slow i2c pull up setting */
		schedule_work(&udc->pullup_job);
}

Canceling udc->pullup_job right after scheduling it prevents pullup_work()
from running if it has not started yet, leaving OTG1_DP_PULLUP asserted in
the transceiver after the UDC is disabled and unbound.  Using flush_work()
here would let the queued pull-up disable complete before tearing down udc.

[Severity: Medium]
This problem wasn't introduced by this patch, but udc_handle_dev() schedules
udc->pullup_job instead of udc->power_job when handling suspend and resume
events [1]:

drivers/usb/gadget/udc/lpc32xx_udc.c:udc_handle_dev() {
    ...
				/* Power down transceiver */
				udc->poweron = 0;
				schedule_work(&udc->pullup_job);
				uda_resm_susp_event(udc, 1);
			}
		} else if ((udc->gadget.speed != USB_SPEED_UNKNOWN) &&
			   udc->driver && udc->vbus) {
			uda_resm_susp_event(udc, 0);
			/* Power up transceiver */
			udc->poweron = 1;
			schedule_work(&udc->pullup_job);
		}
}

Because pullup_work() ignores udc->poweron and udc->power_job is never
scheduled anywhere else, power_work() never runs and the ISP1301 transceiver
never transitions power states across suspend and resume.

[1] https://sashiko.dev/#/bug/linux-931b6f24-8f05-4e96-9acb-90ce7f88f38f

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

[PATCH v2] usb: gadget: lpc32xx_udc: drain the pullup work on removal

[Hyunjung Ko]

isp1301_pullup_enable() schedules udc->pullup_job to perform the slow
isp1301 i2c pull-up access from process context. The work handler
(pullup_work) dereferences udc and touches the isp1301 i2c client.

udc is allocated with devm_kmemdup(), so devres frees it after
lpc32xx_udc_remove() returns. remove() calls pullup(udc, 0) -- which
queues pullup_job to clear the D+ pull-up -- but never drains it, so the
work can run after udc has been freed (and the isp1301 client put),
dereferencing freed memory -- a use-after-free.

Flush pullup_job in lpc32xx_udc_remove() after the final pullup(udc, 0).
flush_work() (rather than cancel_work_sync()) lets the queued pull-up
disable actually complete before the udc and its isp1301 client are
released, so the transceiver is left with D+ deasserted.

Found by static analysis; not runtime-reproduced (needs LPC32xx USB
gadget hardware with the isp1301 transceiver).

Fixes: 24a28e428351 ("USB: gadget driver for LPC32xx")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2: flush_work() instead of cancel_work_sync(), so the pull-up disable
    queued by pullup(udc, 0) completes rather than being dropped
    (OTG1_DP_PULLUP would otherwise stay asserted after unbind) -- per
    Sashiko review of v1.

 drivers/usb/gadget/udc/lpc32xx_udc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
index 94355d3d930f..b98faf5b71e5 100644
--- a/drivers/usb/gadget/udc/lpc32xx_udc.c
+++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
@@ -3187,6 +3187,13 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
 	udc_disable(udc);
 	pullup(udc, 0);

+	/*
+	 * pullup(udc, 0) queues pullup_job to clear the D+ pull-up over i2c;
+	 * flush it so that disable completes and no work can run after the
+	 * devm-allocated udc (and its isp1301 i2c client) are released.
+	 */
+	flush_work(&udc->pullup_job);
+
 	device_init_wakeup(&pdev->dev, 0);
 	remove_debug_file(udc);

--
2.43.0

Re: [PATCH v2] usb: gadget: lpc32xx_udc: drain the pullup work on removal

[sashiko-bot]

> isp1301_pullup_enable() schedules udc->pullup_job to perform the slow
> isp1301 i2c pull-up access from process context. The work handler
> (pullup_work) dereferences udc and touches the isp1301 i2c client.
> 
> udc is allocated with devm_kmemdup(), so devres frees it after
> lpc32xx_udc_remove() returns. remove() calls pullup(udc, 0) -- which
> queues pullup_job to clear the D+ pull-up -- but never drains it, so the
> work can run after udc has been freed (and the isp1301 client put),
> dereferencing freed memory -- a use-after-free.
> 
> Flush pullup_job in lpc32xx_udc_remove() after the final pullup(udc, 0).
> flush_work() (rather than cancel_work_sync()) lets the queued pull-up
> disable actually complete before the udc and its isp1301 client are
> released, so the transceiver is left with D+ deasserted.
> 
> Found by static analysis; not runtime-reproduced (needs LPC32xx USB
> gadget hardware with the isp1301 transceiver).
> 
> Fixes: 24a28e428351 ("USB: gadget driver for LPC32xx")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

Sashiko has reviewed this patch and found no issues.

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