Re: use-after-free in usbnet

Hi Huajun,

On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei [off-list ref] wrote:

quoted

Just skip trying this per your following email's comments.

I will prepare a new patch later, if you'd like to try it.

The below patch reverts the below commits:

       0956a8c20b23d429e79ff86d4325583fc06f9eb4
      (usbnet: increase URB reference count before usb_unlink_urb)

      4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
      (net/usbnet: avoid recursive locking in usbnet_stop())

and keep holding tx/rx queue lock during unlinking, but avoid
to acquire the same queue lock inside complete handler triggered by
usb_unlink_urb.

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..effb34e 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct

sk_buff *skb, struct sk_buff_hea
 {
 	unsigned long		flags;

-	spin_lock_irqsave(&list->lock, flags);
+	if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+		spin_lock_irqsave(&list->lock, flags);
 	__skb_unlink(skb, list);
-	spin_unlock(&list->lock);
+	if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+		spin_unlock(&list->lock);
 	spin_lock(&dev->done.lock);
 	__skb_queue_tail(&dev->done, skb);
 	if (dev->done.qlen == 1)

@@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct

urb *urb, gfp_t flags)
 	usb_fill_bulk_urb (urb, dev->udev, dev->in,
 		skb->data, size, rx_complete, skb);

-	spin_lock_irqsave (&dev->rxq.lock, lockflags);
+	if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+		spin_lock_irqsave (&dev->rxq.lock, lockflags);

 	if (netif_running (dev->net) &&
 	    netif_device_present (dev->net) &&

@@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct

urb *urb, gfp_t flags)
 		netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
 		retval = -ENOLINK;
 	}
-	spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
+	if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
+		spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
 	if (retval) {
 		dev_kfree_skb_any (skb);
 		usb_free_urb (urb);

@@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct

sk_buff_head *q)
 	int			count = 0;

 	spin_lock_irqsave (&q->lock, flags);
+	set_cpu_bit(URB_UNLINKING, dev->cpuflags);
 	skb_queue_walk_safe(q, skb, skbnext) {
 		struct skb_data		*entry;
 		struct urb		*urb;

@@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,

struct sk_buff_head *q)
 		entry = (struct skb_data *) skb->cb;
 		urb = entry->urb;

-		/*
-		 * Get reference count of the URB to avoid it to be
-		 * freed during usb_unlink_urb, which may trigger
-		 * use-after-free problem inside usb_unlink_urb since
-		 * usb_unlink_urb is always racing with .complete
-		 * handler(include defer_bh).
-		 */
-		usb_get_urb(urb);
-		spin_unlock_irqrestore(&q->lock, flags);
 		// during some PM-driven resume scenarios,
 		// these (async) unlinks complete immediately
 		retval = usb_unlink_urb (urb);

@@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct

sk_buff_head *q)
 			netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
 		else
 			count++;
-		usb_put_urb(urb);
-		spin_lock_irqsave(&q->lock, flags);
 	}
+	clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
 	spin_unlock_irqrestore (&q->lock, flags);
 	return count;
 }

@@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
 	usb_kill_urb(dev->interrupt);
 	usb_free_urb(dev->interrupt);

+	free_percpu(dev->cpuflags);
 	free_netdev(net);
 	usb_put_dev (xdev);
 }

@@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const

struct usb_device_id *prod)
 	SET_NETDEV_DEV(net, &udev->dev);

 	dev = netdev_priv(net);
+
+	dev->cpuflags = alloc_percpu(unsigned long);
+	if (!dev->cpuflags) {
+		status = -ENOMEM;
+		goto out1;
+	}
+
 	dev->udev = xdev;
 	dev->intf = udev;
 	dev->driver_info = info;

@@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const

struct usb_device_id *prod)
 	if (info->bind) {
 		status = info->bind (dev, udev);
 		if (status < 0)
-			goto out1;
+			goto out2;

 		// heuristic:  "usb%d" for links we know are two-host,
 		// else "eth%d" when there's reasonable doubt.  userspace

@@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const

struct usb_device_id *prod)
 out3:
 	if (info->unbind)
 		info->unbind (dev, udev);
+out2:
+	free_percpu(dev->cpuflags);
 out1:
 	free_netdev(net);
 out:

diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 605b0aa..2dc46f5 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h

@@ -69,8 +69,28 @@ struct usbnet {
 #		define EVENT_DEV_WAKING 6
 #		define EVENT_DEV_ASLEEP 7
 #		define EVENT_DEV_OPEN	8
+	unsigned long __percpu *cpuflags;
+#		define URB_UNLINKING	0
 };

+static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+	unsigned long *fl = __this_cpu_ptr(addr);
+	set_bit(nr, fl);
+}
+
+static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+	unsigned long *fl = __this_cpu_ptr(addr);
+	clear_bit(nr, fl);
+}
+
+static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+	unsigned long *fl = __this_cpu_ptr(addr);
+	return test_bit(nr, fl);
+}
+
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
 {
 	return to_usb_driver(intf->dev.driver);

Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html