Re: [PATCH v17 7/7] usb: dwc3: rockchip: fix USB-C reconnect in gadget mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: dwc3: rockchip: UAF in vbus handler during gadget teardown
- [High] usb: dwc3: rockchip: unbalanced pm_runtime_put when PM disabled
- [High] usb: dwc3: hardware abort due to late PHY callback unregistration
--

commit d44e5645f925209462f1bf666f95eb79ad1d50f1
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: dwc3: rockchip: fix USB-C reconnect in gadget mode

When USB-C is configured in gadget mode and the cable is unplugged
the USB controller is suspended. After plugging in the cable again,
the USB controller stays suspended and thus the port status remains
not-attached.

This commit fixes this by informing the USB core about VBUS updates when
the PHY mode changes.

> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 1f32150caa090..5cb5feec8bd4b 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
[ ... ]
> @@ -25,8 +25,17 @@ struct dwc3_rk_phy_nb {
>  struct dwc3_rockchip {
>  	struct dwc3		dwc;
>  	u8			phy_reset_active;
> +	enum usb_role		role;
>  };
>  
> +static void dwc3_rockchip_vbus_handler(struct dwc3 *dwc, bool present)
> +{
> +	if (!dwc->gadget || !dwc->gadget_driver)
> +		return;
> +
> +	usb_udc_vbus_handler(dwc->gadget, present);
> +}

[Severity: High]
Does this lockless check of dwc->gadget allow a TOCTOU race?

The handler is invoked by dwc3_rockchip_phy_pre_reset(), which is a PHY
reset callback that can run concurrently (for instance, from DP hotplug
events in the rockchip-usbdp driver via rk_udphy_dp_phy_power_on).

If the PHY reset runs concurrently with dwc3_core_exit_mode(), could the
lockless check succeed just before dwc3_gadget_stop() clears
dwc->gadget_driver and dwc3_gadget_exit() frees dwc->gadget?

Would this cause the reset handler to pass a freed dwc->gadget to
usb_udc_vbus_handler(), resulting in a use-after-free?

Would serializing the check and call within spin_lock_irqsave(&dwc->lock,
flags) prevent this race safely?

> +
>  static int dwc3_rockchip_phy_pre_reset(struct dwc3_rockchip *dwc_rk, int port)
>  {
>  	struct dwc3 *dwc = &dwc_rk->dwc;
[ ... ]
> @@ -51,6 +60,8 @@ static int dwc3_rockchip_phy_pre_reset(struct dwc3_rockchip *dwc_rk, int port)
>  	if (!ret)
>  		return ret;

[Severity: High]
When runtime PM is disabled, pm_runtime_get_if_active(dwc->dev) returns
-EINVAL and does not increment usage_count. Since the `if (!ret)` check
evaluates to false for -EINVAL, will execution continue and lead to an
unbalanced pm_runtime_put_autosuspend() later?

Additionally, does this PM check expose the driver to an asynchronous
external abort (kernel panic) due to late PHY callback unregistration?

During dwc3_core_remove(), clocks are disabled via dwc3_core_exit() while
the PM state remains RPM_ACTIVE until pm_runtime_disable() is called.
Because the PHY callback is unregistered via devres (which runs after
remove completes), could a concurrent DP PHY power-on execute this
pre-reset function and access unclocked hardware registers?

>  
> +	dwc3_rockchip_vbus_handler(dwc, false);
> +
>  	/*
>  	 * Assert USB3 PHY soft reset within DWC3 before the external
>  	 * PHY resets. This disconnects the PIPE interface, preventing
[ ... ]
> @@ -87,6 +100,8 @@ static int dwc3_rockchip_phy_post_reset(struct dwc3_rockchip *dwc_rk, int port)
>  	dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	dwc3_rockchip_vbus_handler(dwc, gadget_mode);
> +
>  	pm_runtime_put_autosuspend(dwc->dev);

[Severity: High]
Since this pm_runtime_put_autosuspend() is executed unconditionally, will
it decrement the usage_count even when pm_runtime_get_if_active()
previously returned -EINVAL?

Could this falsely drop the usage count from > 0 to 0 (stealing a reference
from another subsystem component) and lead to the device being runtime
suspended while actively in use?

>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-b4-rockchip-dwc3-rockchip-glue-v17-0-876f96893e2f@collabora.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy