From patchwork Sat Apr 21 07:45:48 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 154203 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id A307CB6FCA for ; Sat, 21 Apr 2012 17:45:53 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752281Ab2DUHpu (ORCPT ); Sat, 21 Apr 2012 03:45:50 -0400 Received: from mail-pz0-f52.google.com ([209.85.210.52]:57950 "EHLO mail-pz0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751753Ab2DUHps (ORCPT ); Sat, 21 Apr 2012 03:45:48 -0400 Received: by dake40 with SMTP id e40so13915212dak.11 for ; Sat, 21 Apr 2012 00:45:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=VWOyLQ/udCX8Ya2LRe712pcS4FZBuMu9tmoUvaAJ20g=; b=zK7EDhbVKpjJAseUcVcRcfeNo03nlzizMzKibllzaBK8p4DhWhyd+ewFDgpo7d0Nfh mVNP3yX2Zx+vtIDcoQBLVN3WWDdO293hwOb/lBgxYIp5UxhB3bBzod5K5uMk6fyJJ9yz m9tu+sufyW2fnylfHg3pbBf24IJ/7+4EdoGge1h3KGa19LKBdb6XR8T8PI6kDLdyxs/P prU47w4xiwqj3nt7pmZstzb53b8UZkDmIPEaSOsstRl5AWLV4kT/Ep7O1WZp2y7TaR+/ TlJr5KlToI+31LKVxl9dH4+wM5GhaiMOOPXrctnohzW7aNm5X2whUhXOsHhj1YzAJKfG tTAQ== MIME-Version: 1.0 Received: by 10.68.194.72 with SMTP id hu8mr4088976pbc.26.1334994348191; Sat, 21 Apr 2012 00:45:48 -0700 (PDT) Received: by 10.142.212.17 with HTTP; Sat, 21 Apr 2012 00:45:48 -0700 (PDT) In-Reply-To: References: <201203221008.46882.oneukum@suse.de> Date: Sat, 21 Apr 2012 15:45:48 +0800 Message-ID: Subject: Re: use-after-free in usbnet From: Ming Lei To: Huajun Li Cc: Oliver Neukum , Alan Stern , Dave Jones , netdev@vger.kernel.org, linux-usb@vger.kernel.org, Fedora Kernel Team Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi Huajun, On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei 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. Thanks, 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) @@ -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 }; +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);