From patchwork Wed Mar 21 16:22:59 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 148036 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 1DFB6B6F62 for ; Thu, 22 Mar 2012 03:23:20 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759570Ab2CUQXL (ORCPT ); Wed, 21 Mar 2012 12:23:11 -0400 Received: from mail-pz0-f46.google.com ([209.85.210.46]:63601 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759609Ab2CUQXA convert rfc822-to-8bit (ORCPT ); Wed, 21 Mar 2012 12:23:00 -0400 Received: by dajr28 with SMTP id r28so1792006daj.19 for ; Wed, 21 Mar 2012 09:23:00 -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=OV15cFTTm51QS51ivcXM0ioXcTlhEOZ/0v3P2JxBWMI=; b=ETbRcdHd7g8SlA9W+8a9PYhVaDN3hs/3pgNbqpgPnqkTb8TUXDzsLj982kSESSFv8c ibIO7g09rNXjjrRAH5Bd6fQAQ2PCYptQMzDZQ+/N0AuGXaetNVnWfB6wqwI6/MKYt0Qx 41LCOjDiAN5E72TsdD/R8xuD85zaU5+Ra8CpQeEAe69JfftjCyTkx38RW+pZUgRTTdQu JH2+uhn7c/mAaJWpLgz1d3KdonTjiPmkH48cxzZ27Fo1Q+KHOjjoUgU6lRQkHJU5hrhe Z5sLK6VtU2T+l4rhHvzNblFJm0fIlN46KlBv0jkiQdFxGeJ5480TcXnqW8cWeJf6Y8cO 50Fg== MIME-Version: 1.0 Received: by 10.68.218.72 with SMTP id pe8mr12717054pbc.45.1332346979902; Wed, 21 Mar 2012 09:22:59 -0700 (PDT) Received: by 10.143.41.13 with HTTP; Wed, 21 Mar 2012 09:22:59 -0700 (PDT) In-Reply-To: References: Date: Thu, 22 Mar 2012 00:22:59 +0800 Message-ID: Subject: Re: use-after-free in usbnet From: Ming Lei To: Alan Stern Cc: netdev@vger.kernel.org, linux-usb@vger.kernel.org, Fedora Kernel Team , Dave Jones Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Mar 22, 2012 at 12:12 AM, Alan Stern wrote: > On Wed, 21 Mar 2012, Ming Lei wrote: > >> On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern wrote: >> > On Wed, 21 Mar 2012, Ming Lei wrote: >> > >> >> Looks it is a general issue about usb_hcd_unlink_urb. >> >> >> >> Alan, how about increasing URB reference count before calling unlink1 >> >> inside usb_hcd_unlink_urb to fix this kind of problem? >> > >> > No, that won't fix the problem.  The URB could complete and be >> > deallocated even before usb_hcd_unlink_urb() is called, so nothing that >> > function can do will prevent the error. >> >> IMO, driver should not call usb_hcd_unlink_urb after urb is freed from >> the driver, >> but this problem is that URB may be freed during usb_hcd_unlink_urb. > > Drivers don't call usb_hcd_unlink_urb; they call usb_unlink_urb.  This > sort of thing can happen: > >        Driver                  Interrupt handler >        ------                  ----------------- >        call usb_unlink_urb >                                URB completion interrupt occurs >                                 call usb_hcd_giveback_urb >                                  completion routine calls usb_free_urb >                                  URB is deallocated >         call usb_hcd_unlink_urb >          try to increment URB's refcount >          oops because URB is gone Got it, thanks for your detailed explanation. >> In fact, it is allowed that usb_free_urb is called inside .complete handler, >> at least as said in Documentation/URB.txt: >> >>          "You may free an urb that you've submitted,..." >> >> So looks reasonable to increase the URB reference count before calling >> unlink1(), just like that done inside usb_hcd_flush_endpoint().  And I >> think it is a general solution for avoiding this kind of problem. > > It will not solve the problem illustrated above.  The driver must avoid > freeing the URB before usb_unlink_urb returns.  In this case, > increasing the refcount around the unlink call would work. Yes, it is the right fix. >> > It is the caller's responsibility to make sure that the URB does not >> > get freed before usb_unlink_urb() or usb_kill_urb() returns.  We >> > probably should mention this in the kerneldoc... >> >> If so, looks it is a bit contrary with Documentation/URB.txt, also >> this may add extra constraint(maybe unnecessary) to the driver. > > It's not contradictory.  You may indeed free an URB that you have > submitted, so long as you don't free it while usb_unlink_urb (or > related routines like usb_kill_urb) is running. Maybe it should be documented. So looks the correct fix should be below: // these (async) unlinks complete immediately @@ -597,6 +597,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); @@ -1028,7 +1029,6 @@ static void tx_complete (struct urb *urb) } usb_autopm_put_interface_async(dev->intf); - urb->dev = NULL; entry->state = tx_done; defer_bh(dev, skb, &dev->txq); } Thanks, diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 4b8b52c..e36a821 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -588,7 +588,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) entry = (struct skb_data *) skb->cb; urb = entry->urb; - + usb_get_urb(urb); spin_unlock_irqrestore(&q->lock, flags); // during some PM-driven resume scenarios,