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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: dwc3: rockchip: VBUS update skipped due to early return on suspended device
- [High] usb: dwc3: rockchip: VBUS updates are lost when no gadget driver is bound
- [High] usb: dwc3: rockchip: TOCTOU use-after-free of dwc->gadget in vbus handler
- [Medium] usb: dwc3: rockchip: data race on dwc_rk->role during PHY reset and role switch
--

commit 955d4b753ff8d796d1e7b3c020a5064ca8b3dd0a
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. Fix 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 17b6c2ca9e192..ba8ffef2f27e4 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
> @@ -25,8 +25,17 @@ struct dwc3_rockchip {
[ ... ]
> +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]
Can dropping physical VBUS events when no gadget driver is bound
desynchronize the UDC core's state from reality?

The UDC core tracks udc->vbus globally and it defaults to true. If the cable
is physically disconnected while no driver is bound, the false update is
dropped. When a user subsequently loads a gadget driver, the UDC core
observes udc->vbus == true and wrongly activates the pullup resistor. Pre-set
role events will often occur before a gadget driver binds, guaranteeing that
these VBUS updates will be incorrectly dropped.

usb_udc_vbus_handler() safely buffers updates without a driver, making this
check unnecessary.

[Severity: High]
Does reading dwc->gadget locklessly here introduce a Time-of-Check to
Time-of-Use (TOCTOU) use-after-free vulnerability?

This handler is invoked asynchronously by the PHY notifier and locklessly reads
dwc->gadget and dwc->gadget_driver. Concurrently, during a DRD role switch or
driver unbind, dwc3_gadget_exit() in drivers/usb/dwc3/gadget.c sets
dwc->gadget_driver to NULL and calls usb_put_gadget(dwc->gadget), freeing it
without setting the pointer to NULL.

If the PHY thread passes the !dwc->gadget_driver check before teardown unbinds
the driver, but is then preempted, could it dereference a freed gadget pointer
when invoking usb_udc_vbus_handler()?

> +
>  static int dwc3_usb3_phy_notify(struct notifier_block *nb,
>  				unsigned long action, void *data)
>  {
> @@ -57,6 +66,8 @@ static int dwc3_usb3_phy_notify(struct notifier_block *nb,
>  		if (!ret)
>  			return NOTIFY_OK;
>  
> +		dwc3_rockchip_vbus_handler(dwc, false);
> +

[Severity: High]
Does this early return completely bypass the newly introduced VBUS handler
logic when the controller is suspended?

When the controller is suspended, pm_runtime_get_if_active(dwc->dev)
returns 0. This causes dwc3_usb3_phy_notify() to return early from
PHY_NOTIFY_PRE_RESET, entirely skipping the newly added
dwc3_rockchip_vbus_handler() call.

Since this patch is trying to fix the case where the controller stays
suspended on cable plug-in, doesn't bailing here cause the patch to fail to
achieve its primary goal?

>  		/*
>  		 * Assert USB3 PHY soft reset within DWC3 before the external
>  		 * PHY resets. This disconnects the PIPE interface, preventing
[ ... ]
>  	case PHY_NOTIFY_POST_RESET:
> @@ -89,6 +101,8 @@ static int dwc3_usb3_phy_notify(struct notifier_block *nb,
>  		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +		dwc3_rockchip_vbus_handler(dwc, dwc_rk->role == USB_ROLE_DEVICE);
> +

[Severity: High]
Because the PHY_NOTIFY_PRE_RESET logic bailed early on a suspended device,
phy_reset_active is false. Thus the PHY_NOTIFY_POST_RESET handler will early
return before this point:

	case PHY_NOTIFY_POST_RESET:
		spin_lock_irqsave(&dwc->lock, flags);
		if (!(dwc_rk->phy_reset_active & BIT(port))) {
			spin_unlock_irqrestore(&dwc->lock, flags);
			return NOTIFY_OK;
		}

Does this also skip the POST_RESET VBUS update, leaving the device
perpetually suspended?

[Severity: Medium]
Is there a data race on dwc_rk->role here?

This reads dwc_rk->role outside of any locking boundary when determining
whether to propagate a true VBUS state.

>  		pm_runtime_put_autosuspend(dwc->dev);
>  		break;
>  	}
> @@ -130,7 +144,16 @@ static int dwc3_rk_phy_register_notifiers(struct dwc3 *dwc)
>  	return devm_add_action_or_reset(dwc->dev, dwc3_rk_phy_unregister_notifiers, dwc_rk);
>  }
>  
> +static void dwc3_rockchip_set_role(struct dwc3 *dwc, enum usb_role role)
> +{
> +	struct dwc3_rockchip *dwc_rk = container_of(dwc, struct dwc3_rockchip, dwc);
> +
> +	dwc_rk->role = role;
> +	dwc3_rockchip_vbus_handler(dwc, role == USB_ROLE_DEVICE);
> +}

[Severity: Medium]
Is there a data race on dwc_rk->role here?

This locklessly mutates the rockchip-specific role state, which could be read
concurrently during a PHY reset. Can this result in inconsistent role
observations that propagate an incorrect VBUS state to the UDC core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-b4-rockchip-dwc3-rockchip-glue-v16-0-126a2e9133c3@collabora.com?part=6

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