| 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
| ||
|
Message-ID: <CACVXFVMZKBm_=wL=-T=7z8hc+KGHHSNsoRGwHOL4gZtfsZJ46g@mail.gmail.com>
Date: Sun, 22 Apr 2012 20:05:47 +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 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.
>
>
>> @@ -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.
>
>> };
>>
>> +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