Re: [PATCH] usb: dwc3: Fix race between dwc3_set_mode and __dwc3_set_mode

Hi,

On Mon, Nov 28, 2022, Sven Peter wrote:

dwc->desired_dr_role is changed by dwc3_set_mode inside a spinlock but then read by __dwc3_set_mode outside of that lock. This can lead to a race condition when very quick successive role switch events happen:

CPU A dwc3_set_mode(DWC3_GCTL_PRTCAP_HOST) // first role switch event spin_lock_irqsave(&dwc->lock, flags); dwc->desired_dr_role = mode; // DWC3_GCTL_PRTCAP_HOST spin_unlock_irqrestore(&dwc->lock, flags); queue_work(system_freezable_wq, &dwc->drd_work);

CPU B __dwc3_set_mode // .... spin_lock_irqsave(&dwc->lock, flags); // desired_dr_role is DWC3_GCTL_PRTCAP_HOST dwc3_set_prtcap(dwc, dwc->desired_dr_role); spin_unlock_irqrestore(&dwc->lock, flags);

CPU A dwc3_set_mode(DWC3_GCTL_PRTCAP_DEVICE) // second event spin_lock_irqsave(&dwc->lock, flags); dwc->desired_dr_role = mode; // DWC3_GCTL_PRTCAP_DEVICE spin_unlock_irqrestore(&dwc->lock, flags);

CPU B (continues running __dwc3_set_mode) switch (dwc->desired_dr_role) { // DWC3_GCTL_PRTCAP_DEVICE // .... case DWC3_GCTL_PRTCAP_DEVICE: // .... ret = dwc3_gadget_init(dwc);

We then have DWC3_GCTL.DWC3_GCTL_PRTCAPDIR = DWC3_GCTL_PRTCAP_HOST and dwc->current_dr_role = DWC3_GCTL_PRTCAP_HOST but initialized the controller in device mode. It's also possible to get into a state where both host and device are intialized at the same time. Fix this race by creating a local copy of desired_dr_role inside __dwc3_set_mode while holding dwc->lock.

Fixes: 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work properly") Signed-off-by: Sven Peter sven@svenpeter.dev

---

drivers/usb/dwc3/core.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 1f348bc867c2..fc38a8b13efa 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -122,21 +122,25 @@ static void __dwc3_set_mode(struct work_struct *work) unsigned long flags; int ret; u32 reg;

• u32 desired_dr_role;

mutex_lock(&dwc->mutex);

• spin_lock_irqsave(&dwc->lock, flags);
• desired_dr_role = dwc->desired_dr_role;
• spin_unlock_irqrestore(&dwc->lock, flags);

pm_runtime_get_sync(dwc->dev); if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG) dwc3_otg_update(dwc, 0);

• if (!dwc->desired_dr_role)

• if (!desired_dr_role) goto out;

• if (dwc->desired_dr_role == dwc->current_dr_role)

• if (desired_dr_role == dwc->current_dr_role) goto out;

• if (dwc->desired_dr_role == DWC3_GCTL_PRTCAP_OTG && dwc->edev)

• if (desired_dr_role == DWC3_GCTL_PRTCAP_OTG && dwc->edev) goto out;

switch (dwc->current_dr_role) { @@ -164,7 +168,7 @@ static void __dwc3_set_mode(struct work_struct *work) */ if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&

• 	dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
  

• 	desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
  

  reg = dwc3_readl(dwc->regs, DWC3_GCTL); reg |= DWC3_GCTL_CORESOFTRESET; dwc3_writel(dwc->regs, DWC3_GCTL, reg);

@@ -184,11 +188,11 @@ static void __dwc3_set_mode(struct work_struct *work) spin_lock_irqsave(&dwc->lock, flags);

• dwc3_set_prtcap(dwc, dwc->desired_dr_role);

• dwc3_set_prtcap(dwc, desired_dr_role);

spin_unlock_irqrestore(&dwc->lock, flags);

• switch (dwc->desired_dr_role) {

• switch (desired_dr_role) { case DWC3_GCTL_PRTCAP_HOST: ret = dwc3_host_init(dwc); if (ret) {

-- 2.25.1

This looks good to me. This should also go on stable.

Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com

Thanks! Thinh