Re: use-after-free in usbnet

On Wed, 21 Mar 2012, Ming Lei wrote:

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

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.

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.

The extra constraint on the driver is indeed necessary.  However the 
driver can avoid complications by using anchors.

Alan Stern

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