From patchwork Mon Mar 27 10:09:26 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arend Van Spriel X-Patchwork-Id: 743754 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 3vs92h30DSz9s03 for ; Mon, 27 Mar 2017 21:18:04 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=broadcom.com header.i=@broadcom.com header.b="Hc8GhlAT"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752059AbdC0KR5 (ORCPT ); Mon, 27 Mar 2017 06:17:57 -0400 Received: from mail-qk0-f182.google.com ([209.85.220.182]:36204 "EHLO mail-qk0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752176AbdC0KRB (ORCPT ); Mon, 27 Mar 2017 06:17:01 -0400 Received: by mail-qk0-f182.google.com with SMTP id p22so33850694qka.3 for ; Mon, 27 Mar 2017 03:16:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=FPsnp6ITnPyC3XQaoYkjXn8Xi6P/L9XtUqXDtxvIjdU=; b=Hc8GhlATT6JypO+pfY9fjIlACvZW14/SkltRUCZqL3gC+z4YsTIOHWUoZtQLF2NS20 n7hIwFYgQ27NXICZK3xqq89IDDmUMUzb1NcmHK207i2dMC1+82lInjVu080Mcfk/juYF e9GYR7zQcv8RGJbBnMA2GrTxt5FbDCxWqAL4c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=FPsnp6ITnPyC3XQaoYkjXn8Xi6P/L9XtUqXDtxvIjdU=; b=ejRHkdg7jkuPM9MsXkUlLzk3OJ1yFDhAhWD59KYG2qSeD+N1YQesPXmKJLnGqMkusS OhT5BF0svp1emRAMW4ksNAGlEviD39XWwXZWiOGrMFJD8i1mlxEdWOaUEtWNqKEHuIGT lzqNpuBRVoFnA3gLIuObCtPLTPpaqJqQ11G1qVzl8GaXDpEUPacp5FR8zJdDvMPXeko4 qnD76sqnsdIq5mDGfjaQ5kG3ZJHNmSolEJe+frxdNiVphOMzaRShBblpd02AU+7S8v8K ubBLdlpO1iU/CrAxoevKH9EHOMD7QB9qtY4vQhLCGq4J00c7S4s5DW9xCeCMlsmb+x+5 hgcg== X-Gm-Message-State: AFeK/H2DrR9Z3k542qDHxVgeUxiACKRMJslnbOruKNFQ9QS6kHQzcJnFj5r8FRum1gdNzLGV X-Received: by 10.55.149.66 with SMTP id x63mr18454184qkd.277.1490609370290; Mon, 27 Mar 2017 03:09:30 -0700 (PDT) Received: from [10.176.68.31] ([192.19.213.250]) by smtp.gmail.com with ESMTPSA id l73sm27079qke.12.2017.03.27.03.09.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 27 Mar 2017 03:09:29 -0700 (PDT) Subject: Re: [4.9.13] brcmf use-after-free on resume To: Daniel J Blueman , Johannes Berg References: Cc: Netdev , linux-wireless , "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" From: Arend Van Spriel Message-ID: <3c013590-258d-1fec-efc5-0ba2ee2736a5@broadcom.com> Date: Mon, 27 Mar 2017 12:09:26 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 27-3-2017 11:24, Arend Van Spriel wrote: > + Johannes > > On 6-3-2017 11:48, Arend Van Spriel wrote: >> + linux-wireless >> >> On 6-3-2017 8:04, Daniel J Blueman wrote: >>> When resuming from suspend with a BCM43602 on Ubuntu 16.04 with >>> 4.9.13, we see use after free [1]. >>> >>> We see the struct cfg80211_ops is accessed in the resume path, after >>> it was previously freed: >>> >>> (gdb) list *(brcmf_cfg80211_attach+0x10b) >>> 0x1d77b is in brcmf_cfg80211_attach >>> (drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6861). >>> 6856 brcmf_err("ndev is invalid\n"); >>> 6857 return NULL; >>> 6858 } >>> 6859 >>> 6860 ops = kmemdup(&brcmf_cfg80211_ops, sizeof(*ops), GFP_KERNEL); >>> 6861 if (!ops) >>> 6862 return NULL; >>> 6863 >>> 6864 ifp = netdev_priv(ndev); >>> 6865 #ifdef CONFIG_PM >>> >>> (gdb) list *(wiphy_resume+0x591) >>> 0xb751 is in wiphy_resume (net/wireless/sysfs.c:133). >>> 128 int ret = 0; >>> 129 >>> 130 /* Age scan results with time spent in suspend */ >>> 131 cfg80211_bss_age(rdev, get_seconds() - rdev->suspend_at); >>> 132 >>> 133 if (rdev->ops->resume) { >>> 134 rtnl_lock(); >>> 135 if (rdev->wiphy.registered) >>> 136 ret = rdev_resume(rdev); >>> 137 rtnl_unlock(); >>> >>> I'm unsure if this relates to the ordering of callbacks processed by >>> dpm_run_callback. >> >> The problem is that our driver can not access the device as it has been >> powered off during suspend. So in the resume we cleanup everything >> calling wiphy_unregister() and wiphy_free(). This means the rdev in >> wiphy_resume() above is already freed. Not sure how to handle this >> properly. Probably we should do a proper rebind. >> >> Regards, >> Arend >> >>> Thanks, >>> Daniel >>> >>> -- [1] >>> >>> BUG: KASAN: use-after-free in wiphy_resume+0x591/0x5a0 [cfg80211] at >>> addr ffff8803fefebb30 >>> Read of size 8 by task kworker/u16:15/3066 >>> CPU: 0 PID: 3066 Comm: kworker/u16:15 Not tainted 4.9.13-debug+ #7 >>> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 1.2.19 12/22/2016 >>> Workqueue: events_unbound async_run_entry_fn >>> ffff8803bffdf9d8 ffffffff880db6e1 ffff88042740ef00 ffff8803fefebb28 >>> ffff8803bffdfa00 ffffffff87a4d941 ffff8803bffdfa98 ffff8803fefebb20 >>> ffff88042740ef00 ffff8803bffdfa88 ffffffff87a4dbda ffff8803fb132360 >>> Call Trace: >>> [] dump_stack+0x85/0xc4 >>> [] kasan_object_err+0x21/0x70 >>> [] kasan_report_error+0x1fa/0x500 >>> [] ? trace_hardirqs_on_caller+0x3fe/0x580 >>> [] ? cfg80211_bss_age+0x9a/0xc0 [cfg80211] >>> [] ? trace_hardirqs_on+0xd/0x10 >>> [] ? wiphy_suspend+0xc70/0xc70 [cfg80211] >>> [] __asan_report_load8_noabort+0x61/0x70 >>> [] ? wiphy_suspend+0xbb0/0xc70 [cfg80211] >>> [] ? wiphy_resume+0x591/0x5a0 [cfg80211] >>> [] wiphy_resume+0x591/0x5a0 [cfg80211] >>> [] ? wiphy_suspend+0xc70/0xc70 [cfg80211] >>> [] dpm_run_callback+0x6e/0x4f0 >>> [] device_resume+0x1c2/0x670 >>> [] async_resume+0x1d/0x50 >>> [] async_run_entry_fn+0xfe/0x610 >>> [] process_one_work+0x716/0x1a50 >>> [] ? process_one_work+0x679/0x1a50 >>> [] ? _raw_spin_unlock_irq+0x3d/0x60 >>> [] ? pwq_dec_nr_in_flight+0x2b0/0x2b0 >>> [] worker_thread+0xe0/0x1460 >>> [] ? process_one_work+0x1a50/0x1a50 >>> [] kthread+0x222/0x2e0 >>> [] ? kthread_park+0x80/0x80 >>> [] ? kthread_park+0x80/0x80 >>> [] ? kthread_park+0x80/0x80 >>> [] ret_from_fork+0x2a/0x40 > > So the wiphy instance is unregistered *and* freed in our > brcmf_pcie_resume() callback. However, the wiphy_resume() for this > instance still comes right after that. I tried several fixes using API > calls into driver core, but to no avail. So I came up with the fix > below. > > Regards, > Arend > --- resending with proper indentation (I hope). diff --git a/net/wireless/core.c b/net/wireless/core.c index d888613..3af9c2e 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -1096,6 +1096,17 @@ void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev, } EXPORT_SYMBOL(cfg80211_stop_iface); +bool cfg80211_dev_in_list_rtnl(struct cfg80211_registered_device *rdev) +{ + struct cfg80211_registered_device *item; + + list_for_each_entry(item, &cfg80211_rdev_list, list) { + if (item == rdev) + return true; + } + return false; +} + static int cfg80211_netdev_notifier_call(struct notifier_block *nb, unsigned long state, void *ptr) { diff --git a/net/wireless/core.h b/net/wireless/core.h index e9afbc7..0407841 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -286,6 +286,7 @@ struct cfg80211_cqm_config { int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, char *newname); +bool cfg80211_dev_in_list_rtnl(struct cfg80211_registered_device *rdev); void ieee80211_set_bitrate_flags(struct wiphy *wiphy); diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c index 16b6b59..e92459f 100644 --- a/net/wireless/sysfs.c +++ b/net/wireless/sysfs.c @@ -129,16 +129,19 @@ static int wiphy_resume(struct device *dev) struct cfg80211_registered_device *rdev = dev_to_rdev(dev); int ret = 0; + rtnl_lock(); + if (!cfg80211_dev_in_list_rtnl(rdev)) + goto done; + /* Age scan results with time spent in suspend */ cfg80211_bss_age(rdev, get_seconds() - rdev->suspend_at); if (rdev->ops->resume) { - rtnl_lock(); if (rdev->wiphy.registered) ret = rdev_resume(rdev); - rtnl_unlock(); } - +done: + rtnl_unlock(); return ret; }