Re: use-after-free in usbnet

On Wed, Mar 21, 2012 at 10:44 PM, Greg KH [off-list ref] wrote:

On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote:

quoted

On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei [off-list ref] wrote:

quoted

Hi,

On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones [off-list ref] wrote:

quoted

We've had two reports of this use after free in Fedora now recently..

Could you provide output of 'dmesg' and 'lsusb -v' from the reported machine?

Looks I have figured out why your problem is triggered.

If the URB being unlinked is freed before usb_put_dev
inside usb_hcd_unlink_urb, the use-after-free will be triggered.
And the below patch[1] should fix the problem.

With the reference counting we have, how can the urb be freed at this
point in time?  Is the driver doing wierd things with the urb reference
counts?

The problem is that the .complete may schedule a tasklet to
free the completed URB. And the .complete may be run inside
unlink path, so the use-after-free will be triggered if the
tasklet is excuted before usb_put_dev inside usb_hcd_unlink_urb.

quoted

Also there is another bug in tx_complete() of usbnet, the line below

                urb->dev = NULL;

should be removed to avoid possible oops or memory leak in unlink path.

Please test the patch if you can reproduce the problem.

[1],

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 59681f0..4f4e028 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

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

sk_buff_head *q)
              spin_unlock_irqrestore(&q->lock, flags);
              // during some PM-driven resume scenarios,
              // these (async) unlinks complete immediately
+             local_bh_disable();
              retval = usb_unlink_urb (urb);
+             local_bh_enable();

That doesn't seem right, as you point out in your follow-up message.
This shouldn't be needed, unless you are doing some really wierd things
with the urb :(

Looks the driver doesn't do any wierd things, as said above.


Thanks,
-- 
Ming Lei