| 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: <CA+v9cxaqL1V3eyv=dhKbqbnXk6wm1qm93DNFbrbwbd1b1X+3rw@mail.gmail.com>
Date: Sun, 22 Apr 2012 10:19:33 +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 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?
> @@ -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.
> };
>
> +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
--
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