From patchwork Fri Apr 20 13:37:40 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: huajun li X-Patchwork-Id: 154043 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 85E0BB704C for ; Fri, 20 Apr 2012 23:37:45 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756514Ab2DTNhn (ORCPT ); Fri, 20 Apr 2012 09:37:43 -0400 Received: from mail-we0-f174.google.com ([74.125.82.174]:53440 "EHLO mail-we0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753850Ab2DTNhm convert rfc822-to-8bit (ORCPT ); Fri, 20 Apr 2012 09:37:42 -0400 Received: by wejx9 with SMTP id x9so6076801wej.19 for ; Fri, 20 Apr 2012 06:37:40 -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=izYVYdfZljj1ub0UW8dHMaC5/b+b8Qfvnd/XiaCtd0o=; b=oAlkVbU6eHk78R0F3mUjd+pqYkKOMW4DBOv6kzHid49gZrMKJ0+BfynrYijHN/3Xmq Si6IFvUegYrKH6ykB7kzjm/Dg+H8jQT2pwzpRHLXpdFjVVRwK+HXzpmY7wnqhy1ZwEtv TfJZz/2HlxMfqyHngwuM4N00xoL52c0aOTGQ69uOjbqE3pHApD1AUFIUdlHhgDQfPm5Z I0iPzvnnY5R4jxGHpWelvyzZ94VGY97rRC63MQE1iaGZOwUL3YteAPDUcsz5ZJwL/tli HBIvYy7hYCTdqBEzesUCUfRIe596JynQaJvfSCtWMOV0tpvSfOO2IukdOh3o7S0QhKUd hPZA== MIME-Version: 1.0 Received: by 10.180.104.65 with SMTP id gc1mr15205484wib.13.1334929060900; Fri, 20 Apr 2012 06:37:40 -0700 (PDT) Received: by 10.223.17.69 with HTTP; Fri, 20 Apr 2012 06:37:40 -0700 (PDT) In-Reply-To: References: <201203221008.46882.oneukum@suse.de> Date: Fri, 20 Apr 2012 21:37:40 +0800 Message-ID: Subject: Re: use-after-free in usbnet From: Huajun Li To: Ming Lei , Oliver Neukum , Alan Stern , Dave Jones Cc: netdev@vger.kernel.org, linux-usb@vger.kernel.org, Fedora Kernel Team , Huajun Li Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Mar 22, 2012 at 5:30 PM, Ming Lei wrote: > On Thu, Mar 22, 2012 at 5:08 PM, Oliver Neukum wrote: >> >> this looks good, but could you add a comment explaining the reason for >> taking a reference? > > OK, I will post a formal one if you have no objection on the below. > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 4b8b52c..febfdce 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -589,6 +589,14 @@ static int unlink_urbs (struct usbnet *dev, > struct sk_buff_head *q) >                entry = (struct skb_data *) skb->cb; >                urb = entry->urb; > > +               /* > +                * Get a 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 > @@ -597,6 +605,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_put_urb(urb); >                spin_lock_irqsave(&q->lock, flags); >        } >        spin_unlock_irqrestore (&q->lock, flags); > > 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: Thanks, --Huajun Li --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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;