Re: use-after-free in usbnet

On Thu, Mar 22, 2012 at 5:08 PM, Oliver Neukum [off-list ref] wrote:

this looks good, but could you add a comment explaining the reason for
taking a reference?

OK, I will post a formal one if you have no objection on the below.

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 4b8b52c..febfdce 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -589,6 +589,14 @@ static int unlink_urbs (struct usbnet *dev,

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

+		/*
+		 * Get a 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

@@ -597,6 +605,7 @@ 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);
 	}
 	spin_unlock_irqrestore (&q->lock, flags);


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

Am Donnerstag, 22. März 2012, 10:30:36 schrieb Ming Lei:

On Thu, Mar 22, 2012 at 5:08 PM, Oliver Neukum [off-list ref] wrote:

quoted

this looks good, but could you add a comment explaining the reason for
taking a reference?

OK, I will post a formal one if you have no objection on the below.

Good patch :-)

	Regards
		Oliver

On Thu, Mar 22, 2012 at 5:30 PM, Ming Lei [off-list ref] wrote:

Above patch has already been integrated to mainline. However, maybe
there still exists another potentail use-after-free issue, here is a
case:
      After release the lock in unlink_urbs(), defer_bh() may move
current skb from rxq/txq to dev->done queue, even cause the skb be
released. Then in next loop cycle, it can't refer to expected skb, and
may Oops again.

To easily reproduce it, in unlink_urbs(), you can delay a short time
after usb_put_urb(urb), then disconnect your device while transferring
data, and repeat it times you will find errors on your screen.

Following is a draft patch to guarantee the queue consistent, and
refer to expected skb in each loop cycle:

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index b7b3f5b..6da0141 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
 static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
 {
 	unsigned long		flags;
-	struct sk_buff		*skb, *skbnext;
+	struct sk_buff		*skb;
 	int			count = 0;

 	spin_lock_irqsave (&q->lock, flags);
-	skb_queue_walk_safe(q, skb, skbnext) {
+	while (!skb_queue_empty(q)) {
 		struct skb_data		*entry;
 		struct urb		*urb;
 		int			retval;

-		entry = (struct skb_data *) skb->cb;
+		skb_queue_walk(q, skb) {
+			entry = (struct skb_data *)skb->cb;
+			if (entry->state == rx_done ||
+				entry->state == tx_done ||
+				entry->state == rx_cleanup)
+				continue;
+			else
+				break;
+		}
+
+		if (skb == (struct sk_buff *)(q))
+			break;
+
 		urb = entry->urb;

Thanks,
--Huajun Li

On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li [off-list ref] wrote:

Above patch has already been integrated to mainline. However, maybe
there still exists another potentail use-after-free issue, here is a
case:
     After release the lock in unlink_urbs(), defer_bh() may move
current skb from rxq/txq to dev->done queue, even cause the skb be
released. Then in next loop cycle, it can't refer to expected skb, and
may Oops again.

Could you explain in a bit detail? Why can't the expected skb be refered
to in next loop?

To easily reproduce it, in unlink_urbs(), you can delay a short time
after usb_put_urb(urb), then disconnect your device while transferring
data, and repeat it times you will find errors on your screen.

Could you post out the error log?

Maybe it is not necessary, if the state has been changed to  rx_done
or tx_done or rx_cleanup, it means the URB referenced to by the skb
has been completed, and the coming usb_unlink_urb can handle the
case correctly.

+                               continue;
+                       else
+                               break;
+               }
+
+               if (skb == (struct sk_buff *)(q))
+                       break;
+
               urb = entry->urb;


Thanks,
--Huajun Li

Thanks,
-- 
Ming Lei