From patchwork Fri Mar 4 22:26:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrey Konovalov X-Patchwork-Id: 592234 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 3CE76140291 for ; Sat, 5 Mar 2016 09:26:45 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=Qjtd0WX/; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760095AbcCDW0k (ORCPT ); Fri, 4 Mar 2016 17:26:40 -0500 Received: from mail-qg0-f54.google.com ([209.85.192.54]:33885 "EHLO mail-qg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759080AbcCDW0j (ORCPT ); Fri, 4 Mar 2016 17:26:39 -0500 Received: by mail-qg0-f54.google.com with SMTP id w104so55352247qge.1; Fri, 04 Mar 2016 14:26:38 -0800 (PST) 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; bh=dBm5HB9krqdsZuKeIMhkOdC+yquM/da0UH0g9Ghz2U4=; b=Qjtd0WX/nyk1C0ORcO4fMlob24KBKCcyQvBkFcF40ecqc5P4BWf5NzXqULP3THY7Jf Ee83EouSRos/h0x9AWldQeFzMb0sMptvV9UQ43KMo+t+hQcQhjJpvTjTh0Yt2TP7HdzS zINXAh7THp2fvhl67Fi9z21YU8n1Ul3Zk3/dv+CWRxOeiDoXzT/oex+G+ifKP6DQpxyS wE6B0PMTWcM48yzAcb7sQTSUEEoVoGLSHSVxd+XaVYsz2q5bHYhq6vlIv0MVCRXukeGD avQC7QT9lLxLx+ZpxJSrTiZAHVdEhcDgnMb2YTVkeojF8/uMXBH14PdMCJjLHq/Kj4ZI SeAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=dBm5HB9krqdsZuKeIMhkOdC+yquM/da0UH0g9Ghz2U4=; b=OvEOo+qneIa3/maS7aEz7eI+Ff5TEk346gUnkuMhbLE64OztJuOCvoF7ZA0p4zuX0T 5+9yOHlDkGQ8hPJi8nwoYNMIXCHac0gxu5XU+MNqkcp5ENV2EgNEa4wAkGTCiS0dtdjt JhLAOOQHliG5YSceeEf9z+xqRY3lcv1uky43yYvyE0HkBKuWB6I5BegsFcObFuoKqzCr OF7Jor1SDlb4MwY/9TL2rOZpqUAoFRXsk8d51r3QXXp/uNFm6FYnGsgp2Ag6w1afBbFM NLq7rTDqLhaU7iNH9Y0mw0hOpB+CbpUq3p4HdXmQhpL/qzqpqKWQ8y3wBo0ErvQJiWkJ 9J5Q== X-Gm-Message-State: AD7BkJKCketDQtZIqgbqXFl1cQ+RVMXEh6SySkReW5+AHtUhUdtaHMbI4RuwGVUzEa69gKfh2Lu6oFRnN/kNvw== MIME-Version: 1.0 X-Received: by 10.140.104.50 with SMTP id z47mr13287559qge.68.1457130397947; Fri, 04 Mar 2016 14:26:37 -0800 (PST) Received: by 10.55.0.204 with HTTP; Fri, 4 Mar 2016 14:26:37 -0800 (PST) In-Reply-To: References: Date: Sat, 5 Mar 2016 01:26:37 +0300 Message-ID: Subject: Re: Possible double-free in the usbnet driver From: Andrey Konovalov To: Linus Torvalds Cc: Oliver Neukum , Greg Kroah-Hartman , Kostya Serebryany , Dmitry Vyukov , Alexander Potapenko , USB list , Network Development Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, Mar 5, 2016 at 12:26 AM, Linus Torvalds wrote: > [ Moving this to proper lists ] > > On Thu, Mar 3, 2016 at 4:19 PM, Andrey Konovalov wrote: >> >> I found another double-free, this time in the usbnet driver. > > Hmm. It doesn't look like a double free to me, at least from the logs > you attached. > >> Whenever the `bind()` function fails (drivers/net/usb/usbnet.c:1676) when >> called from `usbnet_probe()` (and it can fail due to a invalid usb descriptor), >> `free_netdev()` is called for the `net` device (drivers/net/usb/usbnet.c:1772). >> Then, `free_netdev(net)` is called again in `usbnet_disconnect()` >> (drivers/net/usb/usbnet.c:1570) causing a double-free. > > The KASAN report says that it's a use-after-free in the kworker > thread: the net device got free'd at the end of usbnet_probe(), but > some work-struct was apparently active at the time. > > There might be a double free later that isn't in your report, though. > Do you have the data for that? I uploaded full kernel log here: https://gist.github.com/xairy/6a244871959187209fdb My initial idea was that an object is freed by free_netdev(), then allocated somewhere during execution of kworker-related code and then this object gets freed by free_netdev() again and that's why we have such use-after-free reports. That didn't explain the deallocation stack trace in the report, but I thought this was due to a racy-use-after-free. I just added the following debug output: and when I run the vm and connect the device I get: [ 23.672662] cdc_ncm 1-1:1.6: bind() failure [ 23.673447] usbnet_probe(): freeing netdev: ffff88006ab48000 [ 23.675822] usbnet_probe(): freeing netdev: ffff88006ab48000 So this seems to be a double-free (or at least a double free_netdev() call), but the object gets freed twice from usbnet_probe() and not from usbnet_disconnect(), so you're right that the latter doesn't get called. I'm not sure how usbnet_probe() ends up being called twice. > > But I didn't think we even called the disconnect() function if the > "bind()" failed, so I don't think that one should free it. Greg? > > So it *sounds* to me like the usbnet "bind()" routine ended up > returning an error, but doing so *after* it had already activated the > structure somehow. > > Which particular usbnet bind routine is this? There are multiple > sub-drivers for usbnet that all do different things. cdc_ncm_bind() > > For example, it *looks* like the cdc_ncm_bind() will have done a > usbnet_link_change() even if the bind fails. So now we've done a > usbnet_defer_kevent() even though we're failing, and then that sets > the ball rolling to later touch the netdev that we're freeing due to > the failure. > > But I may be *entirely* misreading this thing. > > Anyway, I'm cc'ing the usbnet people who actually know the code (and netdev). > > The proper fix may be to just cancel any work that might have been set > up before freeing. Or maybe that netdev *does* get free'd later some > other way properly. Let's see what the experts on the usbnet driver > say. > > Linus diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 0b0ba7e..f7e1415 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1567,6 +1567,7 @@ void usbnet_disconnect (struct usb_interface *intf) usb_free_urb(dev->interrupt); kfree(dev->padding_pkt); + pr_err("usbnet_disconnect(): freeing netdev: %p\n", net); free_netdev(net); } EXPORT_SYMBOL_GPL(usbnet_disconnect); @@ -1769,6 +1770,7 @@ out3: if (info->unbind) info->unbind (dev, udev); out1: + pr_err("usbnet_probe(): freeing netdev: %p\n", net); free_netdev(net); out: return status;