linux-kernel - [PATCH 3.16 172/328] USB: net2280: Fix erroneous synchronization change

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: <lsq.1544392233.635235091@decadent.org.uk>
Date:   Sun, 09 Dec 2018 21:50:33 +0000
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org,
        "Felipe Balbi" <felipe.balbi@...ux.intel.com>,
        "Alan Stern" <stern@...land.harvard.edu>,
        "D. Ziesche" <dziesche@....com>
Subject: [PATCH 3.16 172/328] USB: net2280: Fix erroneous synchronization
 change

3.16.62-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Alan Stern <stern@...land.harvard.edu>

commit dec3c23c9aa1815f07d98ae0375b4cbc10971e13 upstream.

Commit f16443a034c7 ("USB: gadgetfs, dummy-hcd, net2280: fix locking
for callbacks") was based on a serious misunderstanding.  It
introduced regressions into both the dummy-hcd and net2280 drivers.

The problem in dummy-hcd was fixed by commit 7dbd8f4cabd9 ("USB:
dummy-hcd: Fix erroneous synchronization change"), but the problem in
net2280 remains.  Namely: the ->disconnect(), ->suspend(), ->resume(),
and ->reset() callbacks must be invoked without the private lock held;
otherwise a deadlock will occur when the callback routine tries to
interact with the UDC driver.

This patch largely is a reversion of the relevant parts of
f16443a034c7.  It also drops the private lock around the calls to
->suspend() and ->resume() (something the earlier patch forgot to do).
This is safe from races with device interrupts because it occurs
within the interrupt handler.

Finally, the patch changes where the ->disconnect() callback is
invoked when net2280_pullup() turns the pullup off.  Rather than
making the callback from within stop_activity() at a time when dropping
the private lock could be unsafe, the callback is moved to a point
after the lock has already been dropped.

Signed-off-by: Alan Stern <stern@...land.harvard.edu>
Fixes: f16443a034c7 ("USB: gadgetfs, dummy-hcd, net2280: fix locking for callbacks")
Reported-by: D. Ziesche <dziesche@....com>
Tested-by: D. Ziesche <dziesche@....com>
Signed-off-by: Felipe Balbi <felipe.balbi@...ux.intel.com>
[bwh: Backported to 3.16:
 - Drop inapplicable change to disconnection handling in handle_stat1_irqs()
 - Adjust filename, context]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
--- a/drivers/usb/gadget/net2280.c
+++ b/drivers/usb/gadget/net2280.c
@@ -1399,11 +1399,14 @@ static int net2280_pullup(struct usb_gad
 		writel(tmp | BIT(USB_DETECT_ENABLE), &dev->usb->usbctl);
 	} else {
 		writel(tmp & ~BIT(USB_DETECT_ENABLE), &dev->usb->usbctl);
-		stop_activity(dev, dev->driver);
+		stop_activity(dev, NULL);
 	}
 
 	spin_unlock_irqrestore (&dev->lock, flags);
 
+	if (!is_on && dev->driver)
+		dev->driver->disconnect(&dev->gadget);
+
 	return 0;
 }
 
@@ -1948,8 +1951,11 @@ stop_activity (struct net2280 *dev, stru
 		nuke (&dev->ep [i]);
 
 	/* report disconnect; the driver is already quiesced */
-	if (driver)
+	if (driver) {
+		spin_unlock(&dev->lock);
 		driver->disconnect(&dev->gadget);
+		spin_lock(&dev->lock);
+	}
 
 	usb_reinit (dev);
 }
@@ -2452,6 +2458,8 @@ next_endpoints:
 		| (1 << PCI_RETRY_ABORT_INTERRUPT))
 
 static void handle_stat1_irqs (struct net2280 *dev, u32 stat)
+__releases(dev->lock)
+__acquires(dev->lock)
 {
 	struct net2280_ep	*ep;
 	u32			tmp, num, mask, scratch;
@@ -2494,6 +2502,7 @@ static void handle_stat1_irqs (struct ne
 	tmp = (1 << SUSPEND_REQUEST_CHANGE_INTERRUPT);
 	if (stat & tmp) {
 		writel (tmp, &dev->regs->irqstat1);
+		spin_unlock(&dev->lock);
 		if (stat & (1 << SUSPEND_REQUEST_INTERRUPT)) {
 			if (dev->driver->suspend)
 				dev->driver->suspend (&dev->gadget);
@@ -2504,6 +2513,7 @@ static void handle_stat1_irqs (struct ne
 				dev->driver->resume (&dev->gadget);
 			/* at high speed, note erratum 0133 */
 		}
+		spin_lock(&dev->lock);
 		stat &= ~tmp;
 	}