| Message ID | 20221128161526.79730-1-sven@svenpeter.dev |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: Fix race between dwc3_set_mode and __dwc3_set_mode | expand |
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
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) {
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(-)