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
On 28-3-2017 13:59, Kalle Valo wrote:
> Arend van Spriel <[email protected]> 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
On Tue, 2017-03-28 at 09:11 +0100, Arend van Spriel wrote:
> We got the following use-after-free KASAN report:
>
[...]
Applied.
johannes
Arend Van Spriel <[email protected]> writes:
> On 28-3-2017 13:59, Kalle Valo wrote:
>> Arend van Spriel <[email protected]> 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
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: [email protected] # 4.10.x, 4.9.x
Reported-by: Daniel J Blueman <[email protected]>
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
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
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
Arend Van Spriel <[email protected]> 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: [email protected] # 4.10.x, 4.9.x
> Reported-by: Daniel J Blueman <[email protected]>
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
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
Arend van Spriel <[email protected]> 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
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
> 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
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: [email protected] # 4.10.x, 4.9.x
Reported-by: Daniel J Blueman <[email protected]>
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
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
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
> > > - 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