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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 22 Apr 2012 10:02:07 +0800
From:	Huajun Li <huajun.li.lee@...il.com>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	Oliver Neukum <oneukum@...e.de>,
	Alan Stern <stern@...land.harvard.edu>,
	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 Sat, Apr 21, 2012 at 5:40 PM, Ming Lei <tom.leiming@...il.com> wrote:
> Hi Huajun,
>
> On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
>> On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming@...il.com> wrote:
>>> Hi Huajun,
>>>
>>> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
>>>
>>>> Did we on the same page, could you please review my patch again?
>>>>
>>>> My draft patch was based on current mainline( 3.4.0-rc3)  which had
>>>> already integrated your previous patch. And in my patch, it replaced
>>>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
>>>> = skb->next'  any more.
>>>
>>> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
>>> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
>>> the oops, does it?
>>>
>>
>> No.
>> In each loop, my patch traverse the queue from its head, and it always
>> holds  q->lock when it need refer "skb->next", this can make sure the
>> right skb is not moved out of rxq/txq.
>
> OK, your patch can avoid the oops, sorry for miss the point.
>
>>
>> Can this fix what you concern? If so, IMO, there is no need to revert
>> your previous patch.
>
> But your patch may introduce another problem, in fact, what your patch does
> is basically same with the below change[1]:
>
> So we can find easily that one same URB may be unlinked more than one
> time with your patch because usb_unlink_urb is asynchronous even though
> it behaves synchronously sometimes.
>
> I remembered that is not allowed, at least usb_unlink_urb's comment says so:
>
>          URBs complete only once per submission, and may be canceled only
>          once per submission.
>

Yes, this is a problem should be avoided.

> [1], against 3.4.0-rc3
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..aadf009 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -578,15 +578,19 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
>  static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>  {
>        unsigned long           flags;
> -       struct sk_buff          *skb, *skbnext;
> +       struct sk_buff          *skb;
>        int                     count = 0;
>
>        spin_lock_irqsave (&q->lock, flags);
> -       skb_queue_walk_safe(q, skb, skbnext) {
> +       while (1) {
>                struct skb_data         *entry;
>                struct urb              *urb;
>                int                     retval;
>
> +               skb = q->next;
> +               if (skb == (struct sk_buff *)q)
> +                       break;
> +
>                entry = (struct skb_data *) skb->cb;
>                urb = entry->urb;
>
>
> 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