From patchwork Thu Apr 26 05:02:35 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 155155 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 239B6B6FB9 for ; Thu, 26 Apr 2012 15:03:16 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751937Ab2DZFCh (ORCPT ); Thu, 26 Apr 2012 01:02:37 -0400 Received: from mail-pz0-f51.google.com ([209.85.210.51]:59285 "EHLO mail-pz0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751361Ab2DZFCf convert rfc822-to-8bit (ORCPT ); Thu, 26 Apr 2012 01:02:35 -0400 Received: by dadz8 with SMTP id z8so1031651dad.10 for ; Wed, 25 Apr 2012 22:02:35 -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:content-transfer-encoding; bh=9wdoap1iDsAkR8rpPmHIjQQ14vWVW9YBerlm9d/Su7c=; b=aOMCBNe4WB/vWo54AuAcQl1nmjOtogmPVE7C58+Wj2/TpQhGYFYrSru8A8SUsDrWBa y9ANLYWh8QT5E8KNdfmDFN9lB/AhuElMpCv5dNVluzyBxoB9dEvfvfCUUAwLmSEoNxAX CvAcVs+edOuc87AlI7/w3vi9oPagTfq7RUd17qaPWXG/fI8LDawtqkWl+7WsPdkKWReW SBTY5ovMpCqSwNs8/DuXAdlK0F9N3xvlnVYjVHbXX3QiX1w7cAZJocbbpZpZj5TIe88A 3JGIG80Q3o4qXyCbWBXjpa3oOIL+ITFPQ5MhSJ6BGps6FgCa+cE9SEBcIf9ZXpbrcR8r w7lg== MIME-Version: 1.0 Received: by 10.68.200.68 with SMTP id jq4mr13465345pbc.42.1335416555272; Wed, 25 Apr 2012 22:02:35 -0700 (PDT) Received: by 10.142.246.12 with HTTP; Wed, 25 Apr 2012 22:02:35 -0700 (PDT) In-Reply-To: References: <201203221008.46882.oneukum@suse.de> Date: Thu, 26 Apr 2012 13:02:35 +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 Fri, Apr 20, 2012 at 9:37 PM, Huajun Li 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. > > To easily reproduce it, in unlink_urbs(), you can delay a short time > after usb_put_urb(urb), then disconnect your device while transferring > data, and repeat it times you will find errors on your screen. > > Following is a draft patch to guarantee the queue consistent, and > refer to expected skb in each loop cycle: > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index b7b3f5b..6da0141 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq); >  static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) >  { >        unsigned long           flags; > -       struct sk_buff          *skb, *skbnext; > +       struct sk_buff          *skb; >        int                     count = 0; > >        spin_lock_irqsave (&q->lock, flags); > -       skb_queue_walk_safe(q, skb, skbnext) { > +       while (!skb_queue_empty(q)) { >                struct skb_data         *entry; >                struct urb              *urb; >                int                     retval; > > -               entry = (struct skb_data *) skb->cb; > +               skb_queue_walk(q, skb) { > +                       entry = (struct skb_data *)skb->cb; > +                       if (entry->state == rx_done || > +                               entry->state == tx_done || > +                               entry->state == rx_cleanup) > +                               continue; > +                       else > +                               break; > +               } > + > +               if (skb == (struct sk_buff *)(q)) > +                       break; > + >                urb = entry->urb; > > After thinking about the issue further, the basic idea of your patch is good, but not safe(double unlink, also races on accessing entry->state), so I write a new one based on your patch. Are you OK this new one? Thanks, diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index db99536..dff5e1b 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -281,17 +281,30 @@ int usbnet_change_mtu (struct net_device *net, int new_mtu) } EXPORT_SYMBOL_GPL(usbnet_change_mtu); +/*The caller must hold list->lock*/ +static void __usbnet_queue_skb(struct sk_buff_head *list, + struct sk_buff *newsk, enum skb_state state) +{ + struct skb_data *entry = (struct skb_data *) newsk->cb; + + __skb_queue_tail(list, newsk); + entry->state = state; +} + /*-------------------------------------------------------------------------*/ /* some LK 2.4 HCDs oopsed if we freed or resubmitted urbs from * completion callbacks. 2.5 should have fixed those bugs... */ -static void defer_bh(struct usbnet *dev, struct sk_buff *skb, struct sk_buff_head *list) +static void defer_bh(struct usbnet *dev, struct sk_buff *skb, + struct sk_buff_head *list, enum skb_state state) { unsigned long flags; + struct skb_data *entry = (struct skb_data *) skb->cb; spin_lock_irqsave(&list->lock, flags); + entry->state = state; __skb_unlink(skb, list); spin_unlock(&list->lock); spin_lock(&dev->done.lock); @@ -339,7 +352,6 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) entry = (struct skb_data *) skb->cb; entry->urb = urb; entry->dev = dev; - entry->state = rx_start; entry->length = 0; usb_fill_bulk_urb (urb, dev->udev, dev->in, @@ -371,7 +383,7 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) tasklet_schedule (&dev->bh); break; case 0: - __skb_queue_tail (&dev->rxq, skb); + __usbnet_queue_skb(&dev->rxq, skb, rx_start); } } else { netif_dbg(dev, ifdown, dev->net, "rx: stopped\n"); @@ -422,16 +434,17 @@ static void rx_complete (struct urb *urb) struct skb_data *entry = (struct skb_data *) skb->cb; struct usbnet *dev = entry->dev; int urb_status = urb->status; + enum skb_state state; skb_put (skb, urb->actual_length); - entry->state = rx_done; + state = rx_done; entry->urb = NULL; switch (urb_status) { /* success */ case 0: if (skb->len < dev->net->hard_header_len) { - entry->state = rx_cleanup; + state = rx_cleanup; dev->net->stats.rx_errors++; dev->net->stats.rx_length_errors++; netif_dbg(dev, rx_err, dev->net, @@ -470,7 +483,7 @@ static void rx_complete (struct urb *urb) "rx throttle %d\n", urb_status); } block: - entry->state = rx_cleanup; + state = rx_cleanup; entry->urb = urb; urb = NULL; break; @@ -481,13 +494,13 @@ block: // FALLTHROUGH default: - entry->state = rx_cleanup; + state = rx_cleanup; dev->net->stats.rx_errors++; netif_dbg(dev, rx_err, dev->net, "rx status %d\n", urb_status); break; } - defer_bh(dev, skb, &dev->rxq); + defer_bh(dev, skb, &dev->rxq, state); if (urb) { if (netif_running (dev->net) && @@ -578,16 +591,24 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq); static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) { unsigned long flags; - struct sk_buff *skb, *skbnext; + struct sk_buff *skb; int count = 0; spin_lock_irqsave (&q->lock, flags); - skb_queue_walk_safe(q, skb, skbnext) { + while (!skb_queue_empty(q)) { struct skb_data *entry; struct urb *urb; int retval; - entry = (struct skb_data *) skb->cb; + skb_queue_walk(q, skb) { + entry = (struct skb_data *) skb->cb; + if (entry->state != unlink_start) + break; + } + if (skb == (struct sk_buff *)q) + break; + + entry->state = unlink_start; urb = entry->urb; /* @@ -598,6 +619,10 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) * handler(include defer_bh). */ usb_get_urb(urb); + + /*speedup unlink by blocking resubmit*/ + usb_block_urb(urb); + spin_unlock_irqrestore(&q->lock, flags); // during some PM-driven resume scenarios, // these (async) unlinks complete immediately @@ -606,6 +631,7 @@ 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_unblock_urb(urb); usb_put_urb(urb); spin_lock_irqsave(&q->lock, flags); } @@ -1039,8 +1065,7 @@ static void tx_complete (struct urb *urb) } usb_autopm_put_interface_async(dev->intf); - entry->state = tx_done; - defer_bh(dev, skb, &dev->txq); + defer_bh(dev, skb, &dev->txq, tx_done); } /*-------------------------------------------------------------------------*/ @@ -1096,7 +1121,6 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, entry = (struct skb_data *) skb->cb; entry->urb = urb; entry->dev = dev; - entry->state = tx_start; entry->length = length; usb_fill_bulk_urb (urb, dev->udev, dev->out, @@ -1155,7 +1179,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, break; case 0: net->trans_start = jiffies; - __skb_queue_tail (&dev->txq, skb); + __usbnet_queue_skb(&dev->txq, skb, tx_start); if (dev->txq.qlen >= TX_QLEN (dev)) netif_stop_queue (net); } diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index 605b0aa..76f4396 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -191,7 +191,8 @@ extern void usbnet_cdc_status(struct usbnet *, struct urb *); enum skb_state { illegal = 0, tx_start, tx_done, - rx_start, rx_done, rx_cleanup + rx_start, rx_done, rx_cleanup, + unlink_start };