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: <Pine.LNX.4.44L0.1203211201560.1369-100000@iolanthe.rowland.org>
Date:	Wed, 21 Mar 2012 12:12:16 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Ming Lei <tom.leiming@...il.com>
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 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

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

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