Re: use-after-free in usbnet

[Ming Lei <tom.leiming@gmail.com>]

On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> 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?
>
>
>      unlink_urbs()                                           complete handler
> --------------------------------------
> -------------------------------------------------
>     spin_unlock_irqrestore()
>                                                                  rx_complete()
>                                                                  derver_bh()
>
>  __skb_unlink()
>
>  __skb_queue_tail(&dev->done, skb)   =======> skb is moved to
> dev->done, and can be freed by usbnet_bh()
>      skb_queue_walk_safe()
>                      tmp = skb->next   ===> refer to freed skb

I see the problem, so looks skb_queue_walk_safe is not safe.
I don' know why the 2nd ' tmp = skb->next' in  skb_queue_walk_safe
is needed and it may become unsafe if skb is freed during current loop.

But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
tmp still may become freed after releasing lock.

> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
> be released soon, right? If so, it should avoid calling
> usb_unlink_urb().

Even though you can avoid calling unlink for completed URBs, the skbs
still may be freed in unlink path because complete handler will be triggered
by unlink and the referenced skb may be freed before next loop, so your
patch can't fix the oops.

As far as I can think of, we can hold lock of done queue to forbid skb free
during unlinking. The below patch may fix the problem, are you OK
with it?

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..a9809d4 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
 	struct sk_buff		*skb, *skbnext;
 	int			count = 0;

-	spin_lock_irqsave (&q->lock, flags);
+	spin_lock_irqsave(&dev->done.lock, flags);
+	spin_lock(&q->lock);
 	skb_queue_walk_safe(q, skb, skbnext) {
 		struct skb_data		*entry;
 		struct urb		*urb;
@@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
 		 * handler(include defer_bh).
 		 */
 		usb_get_urb(urb);
-		spin_unlock_irqrestore(&q->lock, flags);
+		spin_unlock(&q->lock);
 		// during some PM-driven resume scenarios,
 		// these (async) unlinks complete immediately
 		retval = usb_unlink_urb (urb);
@@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
struct sk_buff_head *q)
 		else
 			count++;
 		usb_put_urb(urb);
-		spin_lock_irqsave(&q->lock, flags);
+		spin_lock(&q->lock);
 	}
-	spin_unlock_irqrestore (&q->lock, flags);
+	spin_unlock(&q->lock);
+	spin_unlock_irqrestore(&dev->done.lock, flags);
 	return count;
 }



Thanks,
-- 
Ming Lei