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: <20120321144441.GB14043@kroah.com>
Date:	Wed, 21 Mar 2012 07:44:41 -0700
From:	Greg KH <greg@...ah.com>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	Dave Jones <davej@...hat.com>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org,
	Fedora Kernel Team <kernel-team@...oraproject.org>
Subject: Re: use-after-free in usbnet

On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote:
> On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei <tom.leiming@...il.com> wrote:
> > Hi,
> >
> > On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones <davej@...hat.com> wrote:
> >> 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?

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

greg k-h
--
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