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  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:	Sun, 22 Apr 2012 21:15:36 +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 Sun, Apr 22, 2012 at 8:05 PM, Ming Lei <tom.leiming@...il.com> wrote:
> On Sun, Apr 22, 2012 at 10:19 AM, Huajun Li <huajun.li.lee@...il.com> wrote:
>> On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming@...il.com> wrote:
>>> Hi Huajun,
>>>
>>> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@...il.com> wrote:
>>>>> Just skip trying this per your following email's comments.
>>>>
>>>> I will prepare a new patch later, if you'd like to try it.
>>>
>>> The below patch reverts the below commits:
>>>
>>>       0956a8c20b23d429e79ff86d4325583fc06f9eb4
>>>      (usbnet: increase URB reference count before usb_unlink_urb)
>>>
>>>      4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>>>      (net/usbnet: avoid recursive locking in usbnet_stop())
>>>
>>> and keep holding tx/rx queue lock during unlinking, but avoid
>>> to acquire the same queue lock inside complete handler triggered by
>>> usb_unlink_urb.
>>>
>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>> index db99536..effb34e 100644
>>> --- a/drivers/net/usb/usbnet.c
>>> +++ b/drivers/net/usb/usbnet.c
>>> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
>>> sk_buff *skb, struct sk_buff_hea
>>>  {
>>>        unsigned long           flags;
>>>
>>> -       spin_lock_irqsave(&list->lock, flags);
>>> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> +               spin_lock_irqsave(&list->lock, flags);
>>>        __skb_unlink(skb, list);
>>> -       spin_unlock(&list->lock);
>>> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> +               spin_unlock(&list->lock);
>>>        spin_lock(&dev->done.lock);
>>>        __skb_queue_tail(&dev->done, skb);
>>>        if (dev->done.qlen == 1)
>>
>>
>> Then 'flags' may not be initialized, and this will cause problem while
>> calling spin_unlock_irqrestore(&dev->done.lock, flags), right?
>
> The flag is a percpu variable, so it can't change during the
> above code piece.
>

No, maybe you misunderstood it.
Legacy code stack has 'flags' variable to save irq information, and at
the end defer_bh(), spin_unlock_irqrestore(&dev->done.lock, flags)
will use it. However, the variable may not be initialized in your
patch.

>>
>>
>>> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
>>> urb *urb, gfp_t flags)
>>>        usb_fill_bulk_urb (urb, dev->udev, dev->in,
>>>                skb->data, size, rx_complete, skb);
>>>
>>> -       spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> +               spin_lock_irqsave (&dev->rxq.lock, lockflags);
>>>
>>>        if (netif_running (dev->net) &&
>>>            netif_device_present (dev->net) &&
>>> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
>>> urb *urb, gfp_t flags)
>>>                netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>>>                retval = -ENOLINK;
>>>        }
>>> -       spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
>>> +               spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>>>        if (retval) {
>>>                dev_kfree_skb_any (skb);
>>>                usb_free_urb (urb);
>>> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
>>> sk_buff_head *q)
>>>        int                     count = 0;
>>>
>>>        spin_lock_irqsave (&q->lock, flags);
>>> +       set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>        skb_queue_walk_safe(q, skb, skbnext) {
>>>                struct skb_data         *entry;
>>>                struct urb              *urb;
>>> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
>>> struct sk_buff_head *q)
>>>                entry = (struct skb_data *) skb->cb;
>>>                urb = entry->urb;
>>>
>>> -               /*
>>> -                * Get reference count of the URB to avoid it to be
>>> -                * freed during usb_unlink_urb, which may trigger
>>> -                * use-after-free problem inside usb_unlink_urb since
>>> -                * usb_unlink_urb is always racing with .complete
>>> -                * handler(include defer_bh).
>>> -                */
>>> -               usb_get_urb(urb);
>>> -               spin_unlock_irqrestore(&q->lock, flags);
>>>                // during some PM-driven resume scenarios,
>>>                // these (async) unlinks complete immediately
>>>                retval = usb_unlink_urb (urb);
>>> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
>>> sk_buff_head *q)
>>>                        netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>>                else
>>>                        count++;
>>> -               usb_put_urb(urb);
>>> -               spin_lock_irqsave(&q->lock, flags);
>>>        }
>>> +       clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>>>        spin_unlock_irqrestore (&q->lock, flags);
>>>        return count;
>>>  }
>>> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>>        usb_kill_urb(dev->interrupt);
>>>        usb_free_urb(dev->interrupt);
>>>
>>> +       free_percpu(dev->cpuflags);
>>>        free_netdev(net);
>>>        usb_put_dev (xdev);
>>>  }
>>> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
>>> struct usb_device_id *prod)
>>>        SET_NETDEV_DEV(net, &udev->dev);
>>>
>>>        dev = netdev_priv(net);
>>> +
>>> +       dev->cpuflags = alloc_percpu(unsigned long);
>>> +       if (!dev->cpuflags) {
>>> +               status = -ENOMEM;
>>> +               goto out1;
>>> +       }
>>> +
>>>        dev->udev = xdev;
>>>        dev->intf = udev;
>>>        dev->driver_info = info;
>>> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
>>> struct usb_device_id *prod)
>>>        if (info->bind) {
>>>                status = info->bind (dev, udev);
>>>                if (status < 0)
>>> -                       goto out1;
>>> +                       goto out2;
>>>
>>>                // heuristic:  "usb%d" for links we know are two-host,
>>>                // else "eth%d" when there's reasonable doubt.  userspace
>>> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
>>> struct usb_device_id *prod)
>>>  out3:
>>>        if (info->unbind)
>>>                info->unbind (dev, udev);
>>> +out2:
>>> +       free_percpu(dev->cpuflags);
>>>  out1:
>>>        free_netdev(net);
>>>  out:
>>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>>> index 605b0aa..2dc46f5 100644
>>> --- a/include/linux/usb/usbnet.h
>>> +++ b/include/linux/usb/usbnet.h
>>> @@ -69,8 +69,28 @@ struct usbnet {
>>>  #              define EVENT_DEV_WAKING 6
>>>  #              define EVENT_DEV_ASLEEP 7
>>>  #              define EVENT_DEV_OPEN   8
>>> +       unsigned long __percpu *cpuflags;
>>> +#              define URB_UNLINKING    0
>>
>> Is it possible using a simple bool variable to track whether q->lock
>> is hold by unlink_urb() ?  If yes, it can avoid introducing following
>> new codes into current code stack.
>
> It should be defined as percpu variable. The URB complete handler
> may be triggered inside unlink path, or it can be triggered in hardirq path
> from other CPUs at the same time.
>

Then the potential issue is,  in defer_bh(),  __skb_unlink() may be
called without holding the lock. This can cause current issue too,
because __skb_unlink() may move 'skb->next' out of txq/rxq, and even
free it.

>>
>>>  };
>>>
>>> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
>>> +{
>>> +       unsigned long *fl = __this_cpu_ptr(addr);
>>> +       set_bit(nr, fl);
>>> +}
>>> +
>>> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
>>> +{
>>> +       unsigned long *fl = __this_cpu_ptr(addr);
>>> +       clear_bit(nr, fl);
>>> +}
>>> +
>>> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
>>> +{
>>> +       unsigned long *fl = __this_cpu_ptr(addr);
>>> +       return test_bit(nr, fl);
>>> +}
>>> +
>>>  static inline struct usb_driver *driver_of(struct usb_interface *intf)
>>>  {
>>>        return to_usb_driver(intf->dev.driver);
>>>
>>>
>>> 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

Powered by blists - more mailing lists