Re: use-after-free in usbnet

On Thu, Mar 22, 2012 at 12:12 AM, Alan Stern [off-list ref] wrote:

On Wed, 21 Mar 2012, Ming Lei wrote:

quoted

On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern [off-list ref] wrote:

quoted

On Wed, 21 Mar 2012, Ming Lei wrote:

quoted

Looks it is a general issue about usb_hcd_unlink_urb.

Alan, how about increasing URB reference count before calling unlink1
inside usb_hcd_unlink_urb to fix this kind of problem?

No, that won't fix the problem.  The URB could complete and be
deallocated even before usb_hcd_unlink_urb() is called, so nothing that
function can do will prevent the error.

IMO, driver should not call usb_hcd_unlink_urb after urb is freed from
the driver,
but this problem is that URB may be freed during usb_hcd_unlink_urb.

Drivers don't call usb_hcd_unlink_urb; they call usb_unlink_urb.  This
sort of thing can happen:

       Driver                  Interrupt handler
       ------                  -----------------
       call usb_unlink_urb
                               URB completion interrupt occurs
                                call usb_hcd_giveback_urb
                                 completion routine calls usb_free_urb
                                 URB is deallocated
        call usb_hcd_unlink_urb
         try to increment URB's refcount
         oops because URB is gone

Got it, thanks for your detailed explanation.

quoted

In fact, it is allowed that usb_free_urb is called inside .complete handler,
at least as said in Documentation/URB.txt:

         "You may free an urb that you've submitted,..."

So looks reasonable to increase the URB reference count before calling
unlink1(), just like that done inside usb_hcd_flush_endpoint().  And I
think it is a general solution for avoiding this kind of problem.

It will not solve the problem illustrated above.  The driver must avoid
freeing the URB before usb_unlink_urb returns.  In this case,
increasing the refcount around the unlink call would work.

Yes, it is the right fix.

quoted

quoted

It is the caller's responsibility to make sure that the URB does not
get freed before usb_unlink_urb() or usb_kill_urb() returns.  We
probably should mention this in the kerneldoc...

If so, looks it is a bit contrary with Documentation/URB.txt, also
this may add extra constraint(maybe unnecessary) to the driver.

It's not contradictory.  You may indeed free an URB that you have
submitted, so long as you don't free it while usb_unlink_urb (or
related routines like usb_kill_urb) is running.

Maybe it should be documented.

So looks the correct fix should be below:

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

@@ -588,7 +588,7 @@ static int unlink_urbs (struct usbnet *dev, struct

sk_buff_head *q)

 		entry = (struct skb_data *) skb->cb;
 		urb = entry->urb;
-
+		usb_get_urb(urb);
 		spin_unlock_irqrestore(&q->lock, flags);
 		// during some PM-driven resume scenarios,
 		// these (async) unlinks complete immediately

@@ -597,6 +597,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);

@@ -1028,7 +1029,6 @@ static void tx_complete (struct urb *urb)
 	}

 	usb_autopm_put_interface_async(dev->intf);
-	urb->dev = NULL;
 	entry->state = tx_done;
 	defer_bh(dev, skb, &dev->txq);
 }

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