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: <CACVXFVPfnFLaRFbAgTzQCrPJeaMZ_kEu4q1g-wHesEb2pbdz9w@mail.gmail.com>
Date:	Wed, 21 Mar 2012 23:07:57 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Greg KH <greg@...ah.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 10:44 PM, Greg KH <greg@...ah.com> wrote:
> 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?

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.

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