2023-11-06 15:48:30

by Arend van Spriel

[permalink] [raw]
Subject: Re: [PATCH v5] wifi: brcmfmac: Fix use-after-free bug in brcmf_cfg80211_detach

On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:

> Thanks! I didn't test it for I don't have a device. Very appreciated
> if anyone could help with that.

I would volunteer, but it made me dig deep and not sure if there is a
problem to solve here.

brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
brcmf_notify_escan_complete() which does delete the timer.

What am I missing here?

Regards,
Arend

>
> Kalle Valo <[email protected]> 于2023年11月6日周一 22:41写道:
>>
>> Zheng Wang <[email protected]> writes:
>>
>>> This is the candidate patch of CVE-2023-47233 :
>>> https://nvd.nist.gov/vuln/detail/CVE-2023-47233
>>>
>>> In brcm80211 driver,it starts with the following invoking chain
>>> to start init a timeout worker:
>>>
>>> ->brcmf_usb_probe
>>> ->brcmf_usb_probe_cb
>>> ->brcmf_attach
>>> ->brcmf_bus_started
>>> ->brcmf_cfg80211_attach
>>> ->wl_init_priv
>>> ->brcmf_init_escan
>>> ->INIT_WORK(&cfg->escan_timeout_work,
>>> brcmf_cfg80211_escan_timeout_worker);
>>>
>>> If we disconnect the USB by hotplug, it will call
>>> brcmf_usb_disconnect to make cleanup. The invoking chain is :
>>>
>>> brcmf_usb_disconnect
>>> ->brcmf_usb_disconnect_cb
>>> ->brcmf_detach
>>> ->brcmf_cfg80211_detach
>>> ->kfree(cfg);
>>>
>>> While the timeout woker may still be running. This will cause
>>> a use-after-free bug on cfg in brcmf_cfg80211_escan_timeout_worker.
>>>
>>> Fix it by deleting the timer and canceling the worker in
>>> brcmf_cfg80211_detach.
>>>
>>> Fixes: e756af5b30b0 ("brcmfmac: add e-scan support.")
>>> Signed-off-by: Zheng Wang <[email protected]>
>>> Cc: [email protected]
>>> ---
>>> v5:
>>> - replace del_timer_sync with timer_shutdown_sync suggested by
>>> Arend and Takashi
>>> v4:
>>> - rename the subject and add CVE number as Ping-Ke Shih suggested
>>> v3:
>>> - rename the subject as Johannes suggested
>>> v2:
>>> - fix the error of kernel test bot reported
>>> ---
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> index 667462369a32..a8723a61c9e4 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> @@ -8431,6 +8431,8 @@ void brcmf_cfg80211_detach(struct brcmf_cfg80211_info
>>> *cfg)
>>> if (!cfg)
>>> return;
>>>
>>> + timer_shutdown_sync(&cfg->escan_timeout);
>>> + cancel_work_sync(&cfg->escan_timeout_work);
>>> brcmf_pno_detach(cfg);
>>> brcmf_btcoex_detach(cfg);
>>> wiphy_unregister(cfg->wiphy);
>>
>> Has anyone tested this on a real device? As v1 didn't even compile I am
>> very cautious:
>>
>> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>>
>> --
>> https://patchwork.kernel.org/project/linux-wireless/list/
>>
>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches




Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature