From patchwork Sat Apr 21 01:49:51 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 154188 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 72B19B6FC8 for ; Sat, 21 Apr 2012 11:49:56 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753596Ab2DUBty (ORCPT ); Fri, 20 Apr 2012 21:49:54 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:60274 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752416Ab2DUBtw convert rfc822-to-8bit (ORCPT ); Fri, 20 Apr 2012 21:49:52 -0400 Received: by pbcun15 with SMTP id un15so1671174pbc.19 for ; Fri, 20 Apr 2012 18:49:52 -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=x2gFSZW9R3cMCpm9oxWQVHGQ3Snrscs6wsc13LMq4/g=; b=Y+vBan4hIiHmDq9xau/7gAh+n5Pv+P7PbRinQeYuT1l0Ycv1p734VMuCpjVKIZYVZS e9I5ZnYFEHkSHop86h1Ccz/25T/X2Iy7dG9A4i/Sl9f4IVfcAYWmy054ixZuHCHloMiY I2etK9F7dGzQGE8OuLtrugoYeGStofbrceV4avdMWs/wb1TpHM7HFVEJ28IGi6QPOuLH zyQ4v4BCg2sjogMOOFUPqD8/8aY5nQ/9XzW+ni3hM2jaMFlNqvwqyvw96JttZFZg6pmb a0mRBrMl+nQeK0CIxu/gPWryOPUAwZVwbxSKzRqIRc4pe7hXgd/0qywoFJgNKeau5Z8B O0Aw== MIME-Version: 1.0 Received: by 10.68.201.6 with SMTP id jw6mr17255360pbc.92.1334972991960; Fri, 20 Apr 2012 18:49:51 -0700 (PDT) Received: by 10.142.212.17 with HTTP; Fri, 20 Apr 2012 18:49:51 -0700 (PDT) In-Reply-To: References: <201203221008.46882.oneukum@suse.de> Date: Sat, 21 Apr 2012 09:49:51 +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 On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li wrote: > On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei wrote: >> 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. >> >> Could you explain in a bit detail? Why can't the expected skb be refered >> to in next loop? > > >      unlink_urbs()                                           complete handler > -------------------------------------- > ------------------------------------------------- >     spin_unlock_irqrestore() >                                                                  rx_complete() >                                                                  derver_bh() > >  __skb_unlink() > >  __skb_queue_tail(&dev->done, skb)   =======> skb is moved to > dev->done, and can be freed by usbnet_bh() >      skb_queue_walk_safe() >                      tmp = skb->next   ===> refer to freed skb I see the problem, so looks skb_queue_walk_safe is not safe. I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe is needed and it may become unsafe if skb is freed during current loop. But removing the 2nd 'tmp = skb->next' doesn't help the problem, because tmp still may become freed after releasing lock. > If its state is x_done/tx_done/rx_cleanup, that means the the skb will > be released soon, right? If so, it should avoid calling > usb_unlink_urb(). Even though you can avoid calling unlink for completed URBs, the skbs still may be freed in unlink path because complete handler will be triggered by unlink and the referenced skb may be freed before next loop, so your patch can't fix the oops. As far as I can think of, we can hold lock of done queue to forbid skb free during unlinking. The below patch may fix the problem, are you OK with it? struct urb *urb; @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) * handler(include defer_bh). */ usb_get_urb(urb); - spin_unlock_irqrestore(&q->lock, flags); + spin_unlock(&q->lock); // during some PM-driven resume scenarios, // these (async) unlinks complete immediately retval = usb_unlink_urb (urb); @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) else count++; usb_put_urb(urb); - spin_lock_irqsave(&q->lock, flags); + spin_lock(&q->lock); } - spin_unlock_irqrestore (&q->lock, flags); + spin_unlock(&q->lock); + spin_unlock_irqrestore(&dev->done.lock, flags); return count; } Thanks, diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index db99536..a9809d4 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) struct sk_buff *skb, *skbnext; int count = 0; - spin_lock_irqsave (&q->lock, flags); + spin_lock_irqsave(&dev->done.lock, flags); + spin_lock(&q->lock); skb_queue_walk_safe(q, skb, skbnext) { struct skb_data *entry;