Re: use-after-free in usbnet

On Thu, Apr 26, 2012 at 1:02 PM, Ming Lei [off-list ref] wrote:

Hi Huajun,

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

quoted

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;

After thinking about the issue further, the basic idea of your patch is good,
but not safe(double unlink, also races on accessing entry->state), so I write
a new one based on your patch.

Are you OK this new one?

Yes,  no more issue that I can see by now, thanks.

Thanks,
--
Ming Lei

From 89eb41968caa9523c90c1cf7b7e2259db00e04b8 Mon Sep 17 00:00:00 2001
From: Ming Lei <tom.leiming@gmail.com>
Date: Thu, 26 Apr 2012 11:33:46 +0800
Subject: [PATCH] usbnet: fix skb traversing races during unlink

Commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d(net/usbnet: avoid
recursive locking in usbnet_stop()) fixes the recursive locking
problem by releasing the skb queue lock before unlink, but may
cause skb traversing races:
       - after URB is unlinked and the queue lock is released,
       the refered skb and skb->next may be moved to done queue,
       even be released
       - in skb_queue_walk_safe, the next skb is still obtained
       by next pointer of the last skb
       - so maybe trigger oops or other problems

This patch extends the usage of entry->state to describe 'start_unlink'
state, so always holding the queue(rx/tx) lock to change the state if
the referd skb is in rx or tx queue because we need to know if the
refered urb has been started unlinking in unlink_urbs.

Also the patch uses usb_block_urb introduced by Oliver to block
URB resubmit in complete handler if the URB will be unlinked.

The other part of this patch is based on Hugjun's patch:

s/Hugjun/Huajun

always traverse from head of the tx/rx queue to get skb which is
to be unlinked but not been started unlinking.
---