linux-kernel - [PATCH 3.16 099/178] xhci: Manually give back cancelled URB if we can't queue it for cancel

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.1500213406.781266607@decadent.org.uk>
Date:   Sun, 16 Jul 2017 14:56:46 +0100
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org,
        "Mathias Nyman" <mathias.nyman@...ux.intel.com>,
        diego.viola@...il.com,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Subject: [PATCH 3.16 099/178] xhci: Manually give back cancelled URB if we
 can't queue it for cancel

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

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

From: Mathias Nyman <mathias.nyman@...ux.intel.com>

commit d3519b9d9606991a1305596348b6d690bfa3eb27 upstream.

xhci needs to take care of four scenarios when asked to cancel a URB.

1 URB is not queued or already given back.
  usb_hcd_check_unlink_urb() will return an error, we pass the error on

2 We fail to find xhci internal structures from urb private data such as
  virtual device and endpoint ring.
  Give back URB immediately, can't do anything about internal structures.

3 URB private data has valid pointers to xhci internal data, but host is
  not  responding.
  give back URB immedately and remove the URB from the endpoint lists.

4 Everyting is working
  add URB to cancel list, queue a command to stop the endpoint, after
  which the URB can be turned to no-op or skipped, removed from lists,
  and given back.

We failed to give back the urb in case 2 where the correct device and
endpoint pointers could not be retrieved from URB private data.

This caused a hang on Dell Inspiron 5558/0VNM2T at resume from suspend
as urb was never returned.

[  245.270505] INFO: task rtsx_usb_ms_1:254 blocked for more than 120 seconds.
[  245.272244]       Tainted: G        W       4.11.0-rc3-ARCH #2
[  245.273983] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  245.275737] rtsx_usb_ms_1   D    0   254      2 0x00000000
[  245.277524] Call Trace:
[  245.279278]  __schedule+0x2d3/0x8a0
[  245.281077]  schedule+0x3d/0x90
[  245.281961]  usb_kill_urb.part.3+0x6c/0xa0 [usbcore]
[  245.282861]  ? wake_atomic_t_function+0x60/0x60
[  245.283760]  usb_kill_urb+0x21/0x30 [usbcore]
[  245.284649]  usb_start_wait_urb+0xe5/0x170 [usbcore]
[  245.285541]  ? try_to_del_timer_sync+0x53/0x80
[  245.286434]  usb_bulk_msg+0xbd/0x160 [usbcore]
[  245.287326]  rtsx_usb_send_cmd+0x63/0x90 [rtsx_usb]

Reported-by: diego.viola@...il.com
Tested-by: diego.viola@...il.com
Signed-off-by: Mathias Nyman <mathias.nyman@...ux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
[bwh: Backported to 3.16: adjust context]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
 drivers/usb/host/xhci.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1533,20 +1533,33 @@ int xhci_urb_dequeue(struct usb_hcd *hcd
 	struct xhci_ring *ep_ring;
 	struct xhci_virt_ep *ep;
 	struct xhci_command *command;
+	struct xhci_virt_device *vdev;
 
 	xhci = hcd_to_xhci(hcd);
 	spin_lock_irqsave(&xhci->lock, flags);
 	/* Make sure the URB hasn't completed or been unlinked already */
 	ret = usb_hcd_check_unlink_urb(hcd, urb, status);
-	if (ret || !urb->hcpriv)
+	if (ret)
 		goto done;
+
+	/* give back URB now if we can't queue it for cancel */
+	vdev = xhci->devs[urb->dev->slot_id];
+	urb_priv = urb->hcpriv;
+	if (!vdev || !urb_priv)
+		goto err_giveback;
+
+	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
+	ep = &vdev->eps[ep_index];
+	ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
+	if (!ep || !ep_ring)
+		goto err_giveback;
+
 	temp = readl(&xhci->op_regs->status);
 	if (temp == 0xffffffff || (xhci->xhc_state & XHCI_STATE_HALTED)) {
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 				"HW died, freeing TD.");
-		urb_priv = urb->hcpriv;
 		for (i = urb_priv->td_cnt;
-		     i < urb_priv->length && xhci->devs[urb->dev->slot_id];
+		     i < urb_priv->length;
 		     i++) {
 			td = urb_priv->td[i];
 			if (!list_empty(&td->td_list))
@@ -1554,23 +1567,9 @@ int xhci_urb_dequeue(struct usb_hcd *hcd
 			if (!list_empty(&td->cancelled_td_list))
 				list_del_init(&td->cancelled_td_list);
 		}
-
-		usb_hcd_unlink_urb_from_ep(hcd, urb);
-		spin_unlock_irqrestore(&xhci->lock, flags);
-		usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
-		xhci_urb_free_priv(xhci, urb_priv);
-		return ret;
-	}
-
-	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
-	ep = &xhci->devs[urb->dev->slot_id]->eps[ep_index];
-	ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
-	if (!ep_ring) {
-		ret = -EINVAL;
-		goto done;
+		goto err_giveback;
 	}
 
-	urb_priv = urb->hcpriv;
 	i = urb_priv->td_cnt;
 	if (i < urb_priv->length)
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
@@ -1604,6 +1603,14 @@ int xhci_urb_dequeue(struct usb_hcd *hcd
 done:
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	return ret;
+
+err_giveback:
+	if (urb_priv)
+		xhci_urb_free_priv(xhci, urb_priv);
+	usb_hcd_unlink_urb_from_ep(hcd, urb);
+	spin_unlock_irqrestore(&xhci->lock, flags);
+	usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
+	return ret;
 }
 
 /* Drop an endpoint from a new bandwidth configuration for this device.