Re: [PATCH 3/3] usb: dwc3: support two connectors sharing one HS data bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pm: Unbalanced pm_runtime_put_autosuspend on error path in __dwc3_set_mode
- [High] usb: dwc3: Double-free/UAF of gadget/host resources on GPIO failure in __dwc3_set_mode
- [High] usb: dwc3: deadlock during suspend due to flush_work() on freezable wq
- [Medium] usb: dwc3: Role selection logic implements static priority, contradicting "oldest wins" design
--

commit e9f771770a6e130d4f1859d1ec3e53526b8f1f97
Author: Xu Qianhui <xiaqinghuan@mail.ustc.edu.cn>
Subject: usb: dwc3: support two connectors sharing one HS data bus

This commit introduces support for a single DWC3 controller to drive two
receptacles by muxing the High Speed data lines. It registers one USB role
switch per connector and serializes requests using a mutex, intending to
grant the role to the oldest connector.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index fd5c2cd36c595..76e9e5d1808f0 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -174,8 +178,15 @@ static void __dwc3_set_mode(struct work_struct *work)
>  	mutex_lock(&dwc->mutex);
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	desired_dr_role = dwc->desired_dr_role;
> +	if (mux) {
> +		mux_port = mux->desired_port;
> +		mux_role = mux->desired_role;
> +	}
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	if (mux && READ_ONCE(mux->error))
> +		goto out;

[Severity: High]
If this early error check evaluates to true, it jumps to the out label
before pm_runtime_get_sync() is called. The out label then unconditionally
calls pm_runtime_put_autosuspend(). 

Does this drop a Runtime PM reference that was never acquired, potentially
underflowing the usage count and powering down hardware that is still
actively in use?

> +
>  	pm_runtime_get_sync(dwc->dev);
>  
>  	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG)

[ ... ]

> @@ -233,6 +245,22 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		dwc3_writel(dwc, DWC3_GCTL, reg);
>  	}
>  
> +	/*
> +	 * The previous host/gadget has been torn down above, so it is safe to
> +	 * move the shared HS data path to the requesting connector.
> +	 */
> +	if (mux && mux_port != mux->active_port) {
> +		ret = gpiod_direction_output(mux->select, mux_port);
> +		if (ret) {
> +			WRITE_ONCE(mux->error, ret);
> +			goto out;

[Severity: High]
When gpiod_direction_output() fails, this jumps to the out label after
tearing down the current role, but before dwc3_set_prtcap() is called to
update dwc->current_dr_role.

During subsequent role switches or driver removal, will dwc->current_dr_role
still evaluate to the old role? If so, this could cause dwc3_gadget_exit()
or dwc3_host_exit() to execute a second time, resulting in a double-free of
setup buffers or a use-after-free on device structures.

> +		}
> +
> +		spin_lock_irqsave(&dwc->lock, flags);

[ ... ]

> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index f3e37d383627a..4bf0497e902a7 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -501,10 +502,178 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
>  	return role;
>  }
>  
> +static int dwc3_mux_role_set(struct usb_role_switch *sw, enum usb_role role)
> +{
> +	struct dwc3_role_port *port = usb_role_switch_get_drvdata(sw);
> +	struct dwc3 *dwc = port->dwc;
> +	struct dwc3_role_mux *mux = dwc->role_mux;
> +	enum usb_role selected_role;
> +	unsigned long flags;
> +	int selected, ret;
> +	u32 mode;
> +
> +	if (role < USB_ROLE_NONE || role > USB_ROLE_DEVICE)
> +		return -EINVAL;
> +
> +	mutex_lock(&mux->lock);
> +	if (mux->stopping) {
> +		ret = -ESHUTDOWN;
> +		goto out;
> +	}
> +	if (READ_ONCE(mux->error)) {
> +		ret = READ_ONCE(mux->error);
> +		goto out;
> +	}
> +
> +	port->role = role;
> +	if (mux->ports[0].role != USB_ROLE_NONE)
> +		selected = 0;
> +	else if (mux->ports[1].role != USB_ROLE_NONE)
> +		selected = 1;

[Severity: Medium]
The commit message states that "requests from the two switches are
serialised by a mutex, the oldest connector holding a role wins".

However, this static conditional structure seems to prioritize port 0.
If port 1 is the oldest connector actively holding a role, and port 0
subsequently requests a role, won't this check evaluate port 0 first?
Setting selected = 0 here appears to abruptly preempt port 1 and give
the role to the newer connector, breaking the intended arbitration logic.

> +	else
> +		selected = mux->desired_port;
> +	selected_role = mux->ports[selected].role;
> +	if (selected == mux->desired_port && selected_role == mux->desired_role) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	if (selected_role == USB_ROLE_HOST ||
> +	    (selected_role == USB_ROLE_NONE &&
> +	     dwc->role_switch_default_mode == USB_DR_MODE_HOST))
> +		mode = DWC3_GCTL_PRTCAP_HOST;
> +	else
> +		mode = DWC3_GCTL_PRTCAP_DEVICE;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	mux->desired_port = selected;
> +	mux->desired_role = selected_role;
> +	dwc->desired_dr_role = mode;
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +	queue_work(system_freezable_wq, &dwc->drd_work);
> +	flush_work(&dwc->drd_work);

[Severity: High]
This code queues drd_work on system_freezable_wq and synchronously blocks
on it using flush_work().

Since role switch events can be triggered from non-freezable contexts
(such as ucsi_handle_connector_change running on a non-freezable workqueue),
what happens if a role switch occurs during system suspend after freezable
workqueues have been frozen?

Could flush_work() block indefinitely since drd_work cannot run, leading to
a complete system suspend deadlock?

> +	ret = READ_ONCE(mux->error);
> +out:
> +	mutex_unlock(&mux->lock);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013102927.1141696-1-xiaqinghuan@mail.ustc.edu.cn?part=3