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]
Date:	Sat, 21 Apr 2012 10:02:55 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Huajun Li <huajun.li.lee@...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 9:49 AM, Ming Lei <tom.leiming@...il.com> wrote:
> On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
>> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@...il.com> wrote:
>>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
>>>>
>>>> Above patch has already been integrated to mainline. However, maybe
>>>> there still exists another potentail use-after-free issue, here is a
>>>> case:
>>>>      After release the lock in unlink_urbs(), defer_bh() may move
>>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>>> may Oops again.
>>>
>>> Could you explain in a bit detail? Why can't the expected skb be refered
>>> to in next loop?
>>
>>
>>      unlink_urbs()                                           complete handler
>> --------------------------------------
>> -------------------------------------------------
>>     spin_unlock_irqrestore()
>>                                                                  rx_complete()
>>                                                                  derver_bh()
>>
>>  __skb_unlink()
>>
>>  __skb_queue_tail(&dev->done, skb)   =======> skb is moved to
>> dev->done, and can be freed by usbnet_bh()
>>      skb_queue_walk_safe()
>>                      tmp = skb->next   ===> refer to freed skb
>
> I see the problem, so looks skb_queue_walk_safe is not safe.
> I don' know why the 2nd ' tmp = skb->next' in  skb_queue_walk_safe
> is needed and it may become unsafe if skb is freed during current loop.
>
> But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
> tmp still may become freed after releasing lock.
>
>> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
>> be released soon, right? If so, it should avoid calling
>> usb_unlink_urb().
>
> Even though you can avoid calling unlink for completed URBs, the skbs
> still may be freed in unlink path because complete handler will be triggered
> by unlink and the referenced skb may be freed before next loop, so your
> patch can't fix the oops.
>
> As far as I can think of, we can hold lock of done queue to forbid skb free
> during unlinking. The below patch may fix the problem, are you OK
> with it?

Sorry, the patch still doesn't work since done.lock can't be held
before calling unlinking.

One simple solution is just always holding q->lock during the whole loop,
like before commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
(net/usbnet: avoid recursive locking in usbnet_stop()), and take
the per cpu flag trick to avoid holding q->lock in complete handler
called by unlink, see idea in below link:

http://marc.info/?l=linux-usb&m=133491718707499&w=2

In theory, it should be simple, just take avoiding policy.

>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..a9809d4 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
>        struct sk_buff          *skb, *skbnext;
>        int                     count = 0;
>
> -       spin_lock_irqsave (&q->lock, flags);
> +       spin_lock_irqsave(&dev->done.lock, flags);
> +       spin_lock(&q->lock);
>        skb_queue_walk_safe(q, skb, skbnext) {
>                struct skb_data         *entry;
>                struct urb              *urb;
> @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
>                 * handler(include defer_bh).
>                 */
>                usb_get_urb(urb);
> -               spin_unlock_irqrestore(&q->lock, flags);
> +               spin_unlock(&q->lock);
>                // during some PM-driven resume scenarios,
>                // these (async) unlinks complete immediately
>                retval = usb_unlink_urb (urb);
> @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
> struct sk_buff_head *q)
>                else
>                        count++;
>                usb_put_urb(urb);
> -               spin_lock_irqsave(&q->lock, flags);
> +               spin_lock(&q->lock);
>        }
> -       spin_unlock_irqrestore (&q->lock, flags);
> +       spin_unlock(&q->lock);
> +       spin_unlock_irqrestore(&dev->done.lock, flags);
>        return count;
>  }
>
>
>
> Thanks,
> --
> Ming Lei



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