use-after-free in usbnet

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

 		// 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,

On Thu, 22 Mar 2012, Ming Lei wrote:

> 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);
>  }

Yes, that looks about right.  But I'm not familiar with the details of 
usbnet.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am Mittwoch, 21. März 2012, 17:22:59 schrieb Ming Lei:

> -
> +               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);


Hi,

this looks good, but could you add a comment explaining the reason for
taking a reference?

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html