[PATCH for-4.11 0/2] brcmfmac: fixing use-after-free reports

[PATCH for-4.11 0/2] brcmfmac: fixing use-after-free reports

[Arend van Spriel]

Two use-after-free issues were found using KASAN and reported by
Daniel J Blueman. One of them was submitted as patch. However, no
response came upon my comments. So decided to push the fixes myself.

These patches are intended for v4.11 and apply to the master branch of
the wireless-drivers repository.

Arend van Spriel (2):
  brcmfmac: use local iftype avoiding use-after-free of virtual
    interface
  cfg80211: check rdev resume callback only for registered wiphy

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c |  8 +++++---
 net/wireless/sysfs.c                                   | 10 ++++------
 2 files changed, 9 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH for-4.11 1/2] brcmfmac: use local iftype avoiding use-after-free of virtual interface

[Arend van Spriel]

A use-after-free was found using KASAN. In brcmf_p2p_del_if() the virtual
interface is removed using call to brcmf_remove_interface(). After that
the virtual interface instance has been freed and should not be referenced.
Solve this by storing the nl80211 iftype in local variable, which is used
in a couple of places anyway.

Cc: stable@vger.kernel.org # 4.10.x, 4.9.x
Reported-by: Daniel J Blueman <daniel@quora.org>
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index de19c7c..85d949e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -2238,14 +2238,16 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
 	struct brcmf_p2p_info *p2p = &cfg->p2p;
 	struct brcmf_cfg80211_vif *vif;
+	enum nl80211_iftype iftype;
 	bool wait_for_disable = false;
 	int err;
 
 	brcmf_dbg(TRACE, "delete P2P vif\n");
 	vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
 
+	iftype = vif->wdev.iftype;
 	brcmf_cfg80211_arm_vif_event(cfg, vif);
-	switch (vif->wdev.iftype) {
+	switch (iftype) {
 	case NL80211_IFTYPE_P2P_CLIENT:
 		if (test_bit(BRCMF_VIF_STATUS_DISCONNECTING, &vif->sme_state))
 			wait_for_disable = true;
@@ -2275,7 +2277,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 					    BRCMF_P2P_DISABLE_TIMEOUT);
 
 	err = 0;
-	if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE) {
+	if (iftype != NL80211_IFTYPE_P2P_DEVICE) {
 		brcmf_vif_clear_mgmt_ies(vif);
 		err = brcmf_p2p_release_p2p_if(vif);
 	}
@@ -2291,7 +2293,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	brcmf_remove_interface(vif->ifp, true);
 
 	brcmf_cfg80211_arm_vif_event(cfg, NULL);
-	if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE)
+	if (iftype != NL80211_IFTYPE_P2P_DEVICE)
 		p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif = NULL;
 
 	return err;
-- 
1.9.1

Re: [for-4.11, 1/2] brcmfmac: use local iftype avoiding use-after-free of virtual interface

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
> A use-after-free was found using KASAN. In brcmf_p2p_del_if() the virtual
> interface is removed using call to brcmf_remove_interface(). After that
> the virtual interface instance has been freed and should not be referenced.
> Solve this by storing the nl80211 iftype in local variable, which is used
> in a couple of places anyway.
> 
> Cc: stable@vger.kernel.org # 4.10.x, 4.9.x
> Reported-by: Daniel J Blueman <daniel@quora.org>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Franky Lin <franky.lin@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Patch applied to wireless-drivers.git, thanks.

d77facb88448 brcmfmac: use local iftype avoiding use-after-free of virtual interface

-- 
https://patchwork.kernel.org/patch/9648553/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Arend van Spriel]

We got the following use-after-free KASAN report:

 BUG: KASAN: use-after-free in wiphy_resume+0x591/0x5a0 [cfg80211]
	 at addr ffff8803fc244090
 Read of size 8 by task kworker/u16:24/2587
 CPU: 6 PID: 2587 Comm: kworker/u16:24 Tainted: G    B 4.9.13-debug+
 Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 1.2.19 12/22/2016
 Workqueue: events_unbound async_run_entry_fn
  ffff880425d4f9d8 ffffffffaeedb541 ffff88042b80ef00 ffff8803fc244088
  ffff880425d4fa00 ffffffffae84d7a1 ffff880425d4fa98 ffff8803fc244080
  ffff88042b80ef00 ffff880425d4fa88 ffffffffae84da3a ffffffffc141f7d9
 Call Trace:
  [<ffffffffaeedb541>] dump_stack+0x85/0xc4
  [<ffffffffae84d7a1>] kasan_object_err+0x21/0x70
  [<ffffffffae84da3a>] kasan_report_error+0x1fa/0x500
  [<ffffffffc141f7d9>] ? cfg80211_bss_age+0x39/0xc0 [cfg80211]
  [<ffffffffc141f83a>] ? cfg80211_bss_age+0x9a/0xc0 [cfg80211]
  [<ffffffffae48d46d>] ? trace_hardirqs_on+0xd/0x10
  [<ffffffffc13fb1c0>] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
  [<ffffffffae84def1>] __asan_report_load8_noabort+0x61/0x70
  [<ffffffffc13fb100>] ? wiphy_suspend+0xbb0/0xc70 [cfg80211]
  [<ffffffffc13fb751>] ? wiphy_resume+0x591/0x5a0 [cfg80211]
  [<ffffffffc13fb751>] wiphy_resume+0x591/0x5a0 [cfg80211]
  [<ffffffffc13fb1c0>] ? wiphy_suspend+0xc70/0xc70 [cfg80211]
  [<ffffffffaf3b206e>] dpm_run_callback+0x6e/0x4f0
  [<ffffffffaf3b31b2>] device_resume+0x1c2/0x670
  [<ffffffffaf3b367d>] async_resume+0x1d/0x50
  [<ffffffffae3ee84e>] async_run_entry_fn+0xfe/0x610
  [<ffffffffae3d0666>] process_one_work+0x716/0x1a50
  [<ffffffffae3d05c9>] ? process_one_work+0x679/0x1a50
  [<ffffffffafdd7b6d>] ? _raw_spin_unlock_irq+0x3d/0x60
  [<ffffffffae3cff50>] ? pwq_dec_nr_in_flight+0x2b0/0x2b0
  [<ffffffffae3d1a80>] worker_thread+0xe0/0x1460
  [<ffffffffae3d19a0>] ? process_one_work+0x1a50/0x1a50
  [<ffffffffae3e54c2>] kthread+0x222/0x2e0
  [<ffffffffae3e52a0>] ? kthread_park+0x80/0x80
  [<ffffffffae3e52a0>] ? kthread_park+0x80/0x80
  [<ffffffffae3e52a0>] ? kthread_park+0x80/0x80
  [<ffffffffafdd86aa>] ret_from_fork+0x2a/0x40
 Object at ffff8803fc244088, in cache kmalloc-1024 size: 1024
 Allocated:
 PID = 71
  save_stack_trace+0x1b/0x20
  save_stack+0x46/0xd0
  kasan_kmalloc+0xad/0xe0
  kasan_slab_alloc+0x12/0x20
  __kmalloc_track_caller+0x134/0x360
  kmemdup+0x20/0x50
  brcmf_cfg80211_attach+0x10b/0x3a90 [brcmfmac]
  brcmf_bus_start+0x19a/0x9a0 [brcmfmac]
  brcmf_pcie_setup+0x1f1a/0x3680 [brcmfmac]
  brcmf_fw_request_nvram_done+0x44c/0x11b0 [brcmfmac]
  request_firmware_work_func+0x135/0x280
  process_one_work+0x716/0x1a50
  worker_thread+0xe0/0x1460
  kthread+0x222/0x2e0
  ret_from_fork+0x2a/0x40
 Freed:
 PID = 2568
  save_stack_trace+0x1b/0x20
  save_stack+0x46/0xd0
  kasan_slab_free+0x71/0xb0
  kfree+0xe8/0x2e0
  brcmf_cfg80211_detach+0x62/0xf0 [brcmfmac]
  brcmf_detach+0x14a/0x2b0 [brcmfmac]
  brcmf_pcie_remove+0x140/0x5d0 [brcmfmac]
  brcmf_pcie_pm_leave_D3+0x198/0x2e0 [brcmfmac]
  pci_pm_resume+0x186/0x220
  dpm_run_callback+0x6e/0x4f0
  device_resume+0x1c2/0x670
  async_resume+0x1d/0x50
  async_run_entry_fn+0xfe/0x610
  process_one_work+0x716/0x1a50
  worker_thread+0xe0/0x1460
  kthread+0x222/0x2e0
  ret_from_fork+0x2a/0x40
 Memory state around the buggy address:
  ffff8803fc243f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
  ffff8803fc244000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 >ffff8803fc244080: fc fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                          ^
  ffff8803fc244100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ffff8803fc244180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

What is happening is that brcmf_pcie_resume() detects a device that
is no longer responsive and it decides to unbind resulting in a
wiphy_unregister() and wiphy_free() call. Now the wiphy instance
remains allocated, because PM needs to call wiphy_resume() for it.
However, brcmfmac already does a kfree() for the struct
cfg80211_registered_device::ops field. Changing the checks in
wiphy_resume() to only access the struct cfg80211_registered_device::ops
when the wiphy instance is registered.

Cc: stable@vger.kernel.org # 4.10.x, 4.9.x
Reported-by: Daniel J Blueman <daniel@quora.org>
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 net/wireless/sysfs.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c
index 16b6b59..570a2b6 100644
--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -132,12 +132,10 @@ static int wiphy_resume(struct device *dev)
 	/* 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();
-	}
+	rtnl_lock();
+	if (rdev->wiphy.registered && rdev->ops->resume)
+		ret = rdev_resume(rdev);
+	rtnl_unlock();
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Johannes Berg]

> Changing the checks in
> wiphy_resume() to only access the struct
> cfg80211_registered_device::ops
> when the wiphy instance is registered.

[...]

> +++ b/net/wireless/sysfs.c
> @@ -132,12 +132,10 @@ static int wiphy_resume(struct device *dev)
>  	/* 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();
> -	}
> +	rtnl_lock();
> +	if (rdev->wiphy.registered && rdev->ops->resume)
> +		ret = rdev_resume(rdev);
> +	rtnl_unlock();

Hmm? Commit message seems ... old perhaps?

johannes

Re: [PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Arend Van Spriel]

On 28-3-2017 14:34, Johannes Berg wrote:
>> Changing the checks in
>> wiphy_resume() to only access the struct
>> cfg80211_registered_device::ops
>> when the wiphy instance is registered.
> 
> [...]
> 
>> +++ b/net/wireless/sysfs.c
>> @@ -132,12 +132,10 @@ static int wiphy_resume(struct device *dev)
>>  	/* 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();
>> -	}
>> +	rtnl_lock();
>> +	if (rdev->wiphy.registered && rdev->ops->resume)
>> +		ret = rdev_resume(rdev);
>> +	rtnl_unlock();
> 
> Hmm? Commit message seems ... old perhaps?

Hmmm, why? Before the patch rdev->ops was accessed before checking
rdev->wiphy.registered. When rdev->wiphy.registers is false we no longer
access rdev->ops after the patch. So a driver doing a wiphy_unregister()
can safely kfree() the callback struct after it.

Regards,
Arend

Re: [PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Johannes Berg]

> > > -	if (rdev->ops->resume) {
> > > -		rtnl_lock();
> > > -		if (rdev->wiphy.registered)
> > > -			ret = rdev_resume(rdev);
> > > -		rtnl_unlock();
> > > -	}
> > > +	rtnl_lock();
> > > +	if (rdev->wiphy.registered && rdev->ops->resume)
> > > +		ret = rdev_resume(rdev);
> > > +	rtnl_unlock();
> > 
> > Hmm? Commit message seems ... old perhaps?
> 
> Hmmm, why? Before the patch rdev->ops was accessed before checking
> rdev->wiphy.registered. When rdev->wiphy.registers is false we no
> longer access rdev->ops after the patch. So a driver doing a
> wiphy_unregister() can safely kfree() the callback struct after it.

Oh, right. Looks like I misinterpreted things.

johannes

Re: [PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Arend Van Spriel]

On 28-3-2017 16:25, Johannes Berg wrote:
> 
>>>> -	if (rdev->ops->resume) {
>>>> -		rtnl_lock();
>>>> -		if (rdev->wiphy.registered)
>>>> -			ret = rdev_resume(rdev);
>>>> -		rtnl_unlock();
>>>> -	}
>>>> +	rtnl_lock();
>>>> +	if (rdev->wiphy.registered && rdev->ops->resume)
>>>> +		ret = rdev_resume(rdev);
>>>> +	rtnl_unlock();
>>>
>>> Hmm? Commit message seems ... old perhaps?
>>
>> Hmmm, why? Before the patch rdev->ops was accessed before checking
>> rdev->wiphy.registered. When rdev->wiphy.registers is false we no
>> longer access rdev->ops after the patch. So a driver doing a
>> wiphy_unregister() can safely kfree() the callback struct after it.
> 
> Oh, right. Looks like I misinterpreted things.

So apparently my choice of words was poor. Do you want me to rephrase?

Regards,
Arend

Re: [PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Johannes Berg]

On Tue, 2017-03-28 at 16:46 +0200, Arend Van Spriel wrote:
> 
> On 28-3-2017 16:25, Johannes Berg wrote:
> > 
> > > > > -	if (rdev->ops->resume) {
> > > > > -		rtnl_lock();
> > > > > -		if (rdev->wiphy.registered)
> > > > > -			ret = rdev_resume(rdev);
> > > > > -		rtnl_unlock();
> > > > > -	}
> > > > > +	rtnl_lock();
> > > > > +	if (rdev->wiphy.registered && rdev->ops->resume)
> > > > > +		ret = rdev_resume(rdev);
> > > > > +	rtnl_unlock();
> > > > 
> > > > Hmm? Commit message seems ... old perhaps?
> > > 
> > > Hmmm, why? Before the patch rdev->ops was accessed before
> > > checking
> > > rdev->wiphy.registered. When rdev->wiphy.registers is false we no
> > > longer access rdev->ops after the patch. So a driver doing a
> > > wiphy_unregister() can safely kfree() the callback struct after
> > > it.
> > 
> > Oh, right. Looks like I misinterpreted things.
> 
> So apparently my choice of words was poor. Do you want me to
> rephrase?

Nah, don't worry. When I apply it I'll re-read and see if I just
confused myself or if it makes sense to reword a bit.

johannes

Re: [PATCH for-4.11 2/2] cfg80211: check rdev resume callback only for registered wiphy

[Johannes Berg]

On Tue, 2017-03-28 at 09:11 +0100, Arend van Spriel wrote:
> We got the following use-after-free KASAN report:
> 
[...]

Applied.

johannes

Re: [PATCH for-4.11 0/2] brcmfmac: fixing use-after-free reports

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> Two use-after-free issues were found using KASAN and reported by
> Daniel J Blueman. One of them was submitted as patch. However, no
> response came upon my comments. So decided to push the fixes myself.
>
> These patches are intended for v4.11 and apply to the master branch of
> the wireless-drivers repository.
>
> Arend van Spriel (2):
>   brcmfmac: use local iftype avoiding use-after-free of virtual
>     interface
>   cfg80211: check rdev resume callback only for registered wiphy
>
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c |  8 +++++---
>  net/wireless/sysfs.c                                   | 10 ++++------
>  2 files changed, 9 insertions(+), 9 deletions(-)

Why are these in the same patchset, are there any dependencies etc? Or
is it safe that Johannes applies the cfg80211 patch to mac80211.git and
I apply brcmfmac to wireless-drivers.git?

-- 
Kalle Valo

Re: [PATCH for-4.11 0/2] brcmfmac: fixing use-after-free reports

[Arend Van Spriel]

On 28-3-2017 13:59, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
> 
>> Two use-after-free issues were found using KASAN and reported by
>> Daniel J Blueman. One of them was submitted as patch. However, no
>> response came upon my comments. So decided to push the fixes myself.
>>
>> These patches are intended for v4.11 and apply to the master branch of
>> the wireless-drivers repository.
>>
>> Arend van Spriel (2):
>>   brcmfmac: use local iftype avoiding use-after-free of virtual
>>     interface
>>   cfg80211: check rdev resume callback only for registered wiphy
>>
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c |  8 +++++---
>>  net/wireless/sysfs.c                                   | 10 ++++------
>>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> Why are these in the same patchset, are there any dependencies etc? Or
> is it safe that Johannes applies the cfg80211 patch to mac80211.git and
> I apply brcmfmac to wireless-drivers.git?

Yeah. My bad. I just realized while driving to pick up my son from
school :-p Anyway, these are in the same patchset just because of their
context and independent so you both can apply them in their respective
repository.

Regards,
Arend

Re: [PATCH for-4.11 0/2] brcmfmac: fixing use-after-free reports

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On 28-3-2017 13:59, Kalle Valo wrote:
>> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>> 
>>> Two use-after-free issues were found using KASAN and reported by
>>> Daniel J Blueman. One of them was submitted as patch. However, no
>>> response came upon my comments. So decided to push the fixes myself.
>>>
>>> These patches are intended for v4.11 and apply to the master branch of
>>> the wireless-drivers repository.
>>>
>>> Arend van Spriel (2):
>>>   brcmfmac: use local iftype avoiding use-after-free of virtual
>>>     interface
>>>   cfg80211: check rdev resume callback only for registered wiphy
>>>
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c |  8 +++++---
>>>  net/wireless/sysfs.c                                   | 10 ++++------
>>>  2 files changed, 9 insertions(+), 9 deletions(-)
>> 
>> Why are these in the same patchset, are there any dependencies etc? Or
>> is it safe that Johannes applies the cfg80211 patch to mac80211.git and
>> I apply brcmfmac to wireless-drivers.git?
>
> Yeah. My bad. I just realized while driving to pick up my son from
> school :-p

Hehe, that has happened to me also :)

> Anyway, these are in the same patchset just because of their context
> and independent so you both can apply them in their respective
> repository.

Ok, we'll take them separately. Thanks.

-- 
Kalle Valo