netdev - Re: use-after-free in usbnet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACVXFVOVjnWjqpKxbU98DAyUC_OSb8ZL-3WcyYuFXgPJn5UyuA@mail.gmail.com>
Date:	Thu, 22 Mar 2012 00:22:59 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Fedora Kernel Team <kernel-team@...oraproject.org>,
	Dave Jones <davej@...hat.com>
Subject: Re: use-after-free in usbnet

On Thu, Mar 22, 2012 at 12:12 AM, Alan Stern <stern@...land.harvard.edu> wrote:
> On Wed, 21 Mar 2012, Ming Lei wrote:
>
>> On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern <stern@...land.harvard.edu> wrote:
>> > On Wed, 21 Mar 2012, Ming Lei wrote:
>> >
>> >> 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.

>> 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.

>> > 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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html