[PATCH 1/6] usbip: vhci_hcd: protect urb->hcpriv with priv_lock in vhci_urb_dequeue()

[Raul E Rangel]

In vhci_urb_dequeue(), urb->hcpriv is read and priv->vdev is
dereferenced while holding only vhci->lock, before acquiring
vdev->priv_lock. However, pickup_urb_and_free_priv() in vhci_rx.c frees
priv and clears urb->hcpriv under vdev->priv_lock without holding
vhci->lock.

If vhci_urb_dequeue() races with pickup_urb_and_free_priv() on a
completing URB, vhci_urb_dequeue() can observe a non-NULL priv pointer
just before pickup_urb_and_free_priv() frees it, leading to a
use-after-free when dereferencing priv->vdev or priv->seqnum, or a
double-free of priv.

Resolve the race by deriving vdev from urb->dev->portnum (matching
vhci_urb_enqueue(), and guarding against portnum == 0 in both) and
acquiring vdev->priv_lock (nested inside vhci->lock) before reading
urb->hcpriv.

Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Cc: stable@xxxxxxxxxxxxxxx
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@xxxxxxxxxxxx>
---

 drivers/usb/usbip/vhci_hcd.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 39e8faf4c18c..3196ea80e7c2 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -704,7 +704,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	struct vhci_device *vdev;
 	unsigned long flags;
 
-	if (portnum > VHCI_HC_PORTS) {
+	if (!portnum || portnum > VHCI_HC_PORTS) {
 		dev_err(hcd_dev(hcd), "invalid port number %d\n", portnum);
 		return -ENODEV;
 	}
@@ -899,34 +899,35 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	struct vhci_priv *priv;
 	struct vhci_device *vdev;
 	unsigned long flags;
+	u8 portnum = urb->dev->portnum;
+	int ret;
+
+	if (!portnum || portnum > VHCI_HC_PORTS)
+		return -ENODEV;
+	vdev = &vhci_hcd->vdev[portnum - 1];
 
 	spin_lock_irqsave(&vhci->lock, flags);
+	spin_lock(&vdev->priv_lock);
 
 	priv = urb->hcpriv;
 	if (!priv) {
 		/* URB was never linked! or will be soon given back by
 		 * vhci_rx. */
+		spin_unlock(&vdev->priv_lock);
 		spin_unlock_irqrestore(&vhci->lock, flags);
 		return -EIDRM;
 	}
 
-	{
-		int ret = 0;
-
-		ret = usb_hcd_check_unlink_urb(hcd, urb, status);
-		if (ret) {
-			spin_unlock_irqrestore(&vhci->lock, flags);
-			return ret;
-		}
+	ret = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (ret) {
+		spin_unlock(&vdev->priv_lock);
+		spin_unlock_irqrestore(&vhci->lock, flags);
+		return ret;
 	}
 
 	 /* send unlink request here? */
-	vdev = priv->vdev;
-
 	if (!vdev->ud.tcp_socket) {
 		/* tcp connection is closed */
-		spin_lock(&vdev->priv_lock);
-
 		list_del(&priv->list);
 		kfree(priv);
 		urb->hcpriv = NULL;
@@ -948,8 +949,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		/* tcp connection is alive */
 		struct vhci_unlink *unlink;
 
-		spin_lock(&vdev->priv_lock);
-
 		/* setup CMD_UNLINK pdu */
 		unlink = kzalloc_obj(struct vhci_unlink, GFP_ATOMIC);
 		if (!unlink) {
-- 
2.56.0.212.g129ede20d9-goog