Return-path: Received: from mail-qk0-f174.google.com ([209.85.220.174]:36140 "EHLO mail-qk0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752333AbdC0KTF (ORCPT ); Mon, 27 Mar 2017 06:19:05 -0400 Received: by mail-qk0-f174.google.com with SMTP id p22so33891667qka.3 for ; Mon, 27 Mar 2017 03:19:04 -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> (sfid-20170327_121957_798857_90E2343D) Date: Mon, 27 Mar 2017 12:09:26 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: 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; }