Re: use-after-free in usbnet

[Huajun Li <huajun.li.lee@xxxxxxxxx>]

On Thu, Mar 22, 2012 at 5:30 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> On Thu, Mar 22, 2012 at 5:08 PM, Oliver Neukum <oneukum@xxxxxxx> 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);
>
>


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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html