linux-kernel - [PATCH 4.4 007/312] usb: gadget: pch_udc: reorder spin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200508123125.033701501@linuxfoundation.org>
Date:   Fri,  8 May 2020 14:29:58 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        stable@...r.kernel.org, Iago Abal <mail@...oabal.eu>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>
Subject: [PATCH 4.4 007/312] usb: gadget: pch_udc: reorder spin_[un]lock to avoid deadlock

From: Iago Abal <mail@...oabal.eu>

commit 1d23d16a88e6c8143b07339435ba061b131ebb8c upstream.

The above commit reordered spin_lock/unlock and now `&dev->lock' is acquired
(rather than released) before calling `dev->driver->disconnect',
`dev->driver->setup', `dev->driver->suspend', `usb_gadget_giveback_request', and
`usb_gadget_udc_reset'.

But this *may* not be the right way to fix the problem pointed by d3cb25a12138.

Note that the other usb/gadget/udc drivers do release the lock before calling
these functions. There are also inconsistencies within pch_udc.c, where
`dev->driver->disconnect' is called while holding `&dev->lock' in lines 613 and
1184, but not in line 2739.

Finally, commit d3cb25a12138 may have introduced several potential deadlocks.

For instance, EBA (https://github.com/models-team/eba) reports:

    Double lock in drivers/usb/gadget/udc/pch_udc.c
    first at 2791: spin_lock(& dev->lock); [pch_udc_isr]
    second at 2694: spin_lock(& dev->lock); [pch_udc_svc_cfg_interrupt]
        after calling from 2793: pch_udc_dev_isr(dev, dev_intr);
        after calling from 2724: pch_udc_svc_cfg_interrupt(dev);

Similarly, other potential deadlocks are 2791 -> 2793 -> 2721 -> 2657; and
2791 -> 2793 -> 2711 -> 2573 -> 1499 -> 1480.

Fixes: d3cb25a12138 ("usb: gadget: udc: fix spin_lock in pch_udc")
Signed-off-by: Iago Abal <mail@...oabal.eu>
Signed-off-by: Felipe Balbi <felipe.balbi@...ux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>

---
 drivers/usb/gadget/udc/pch_udc.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1488,11 +1488,11 @@ static void complete_req(struct pch_udc_
 		req->dma_mapped = 0;
 	}
 	ep->halted = 1;
-	spin_lock(&dev->lock);
+	spin_unlock(&dev->lock);
 	if (!ep->in)
 		pch_udc_ep_clear_rrdy(ep);
 	usb_gadget_giveback_request(&ep->ep, &req->req);
-	spin_unlock(&dev->lock);
+	spin_lock(&dev->lock);
 	ep->halted = halted;
 }
 
@@ -2583,9 +2583,9 @@ static void pch_udc_svc_ur_interrupt(str
 		empty_req_queue(ep);
 	}
 	if (dev->driver) {
-		spin_lock(&dev->lock);
-		usb_gadget_udc_reset(&dev->gadget, dev->driver);
 		spin_unlock(&dev->lock);
+		usb_gadget_udc_reset(&dev->gadget, dev->driver);
+		spin_lock(&dev->lock);
 	}
 }
 
@@ -2664,9 +2664,9 @@ static void pch_udc_svc_intf_interrupt(s
 		dev->ep[i].halted = 0;
 	}
 	dev->stall = 0;
-	spin_lock(&dev->lock);
-	dev->driver->setup(&dev->gadget, &dev->setup_data);
 	spin_unlock(&dev->lock);
+	dev->driver->setup(&dev->gadget, &dev->setup_data);
+	spin_lock(&dev->lock);
 }
 
 /**
@@ -2701,9 +2701,9 @@ static void pch_udc_svc_cfg_interrupt(st
 	dev->stall = 0;
 
 	/* call gadget zero with setup data received */
-	spin_lock(&dev->lock);
-	dev->driver->setup(&dev->gadget, &dev->setup_data);
 	spin_unlock(&dev->lock);
+	dev->driver->setup(&dev->gadget, &dev->setup_data);
+	spin_lock(&dev->lock);
 }
 
 /**