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 zyytlz.wz@163.com Cc: stable@vger.kernel.org --- 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);
Zheng Wang zyytlz.wz@163.com 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 zyytlz.wz@163.com Cc: stable@vger.kernel.org
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/20231104054709.716...
Thanks! I didn't test it for I don't have a device. Very appreciated if anyone could help with that.
Kalle Valo kvalo@kernel.org 于2023年11月6日周一 22:41写道：
Zheng Wang zyytlz.wz@163.com 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 zyytlz.wz@163.com Cc: stable@vger.kernel.org
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/20231104054709.716...
-- https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatch...
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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 kvalo@kernel.org 于2023年11月6日周一 22:41写道：
Zheng Wang zyytlz.wz@163.com 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 zyytlz.wz@163.com Cc: stable@vger.kernel.org
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/20231104054709.716...
-- https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatch...
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time. As I'm not very familiar with the logic here. I'm still not sure if we should delete the timer_shutdown_sync. Looking forward to your reply :)
```cpp if (cfg->int_escan_map || cfg->scan_request) { escan->escan_state = WL_ESCAN_STATE_IDLE; brcmf_notify_escan_complete(cfg, escan->ifp, true, true); } ```
Best regards, Zheng
Regards, Arend
Kalle Valo kvalo@kernel.org 于2023年11月6日周一 22:41写道：
Zheng Wang zyytlz.wz@163.com 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 zyytlz.wz@163.com Cc: stable@vger.kernel.org
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/20231104054709.716...
-- https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatch...
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
As I'm not very familiar with the logic here. I'm still not sure if we should delete the timer_shutdown_sync. Looking forward to your reply :)
Reading the kerneldoc of timer_shutdown_sync() has the advantage that the timer can not be rearmed by another thread. However, that will only happen when a new scan is initiated in firmware, but the bus is already down so that can not happen. The only improvement (no bug fix!) I see here is to replace timer handling code in brcmf_notify_escan_complete():
- if (timer_pending(&cfg->escan_timeout)) - del_timer_sync(&cfg->escan_timeout); + timer_delete_sync(&cfg->escan_timeout);
Regards, Arend
Arend van Spriel arend.vanspriel@broadcom.com writes:
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
TBH I don't take CVE entries seriously anymore. I don't know what has happened there.
Arend van Spriel arend.vanspriel@broadcom.com 于2023年11月13日周一 17:18写道：
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
I'm sure the issue exists because there's only cancler of timer but not woker. As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218, I submit it as soon as I found it.
As I'm not very familiar with the logic here. I'm still not sure if we should delete the timer_shutdown_sync. Looking forward to your reply :)
Reading the kerneldoc of timer_shutdown_sync() has the advantage that the timer can not be rearmed by another thread. However, that will only happen when a new scan is initiated in firmware, but the bus is already down so that can not happen. The only improvement (no bug fix!) I see here is to replace timer handling code in brcmf_notify_escan_complete():
if (timer_pending(&cfg->escan_timeout))del_timer_sync(&cfg->escan_timeout);
timer_delete_sync(&cfg->escan_timeout);
Very thanks for your reviews and suggestions! I thinks it's a good idea. I'll make another patch sooner or later.
Best regards, Zheng
Regards, Arend
On November 15, 2023 4:00:46 PM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend van Spriel arend.vanspriel@broadcom.com 于2023年11月13日周一 17:18写道：
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
I'm sure the issue exists because there's only cancler of timer but not woker. As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218, I submit it as soon as I found it.
Ah, yes. The cancel_work_sync() can also be done in brcmf_notify_escan_complete().
Regards, Arend
On Thu, 16 Nov 2023 19:20:06 +0100, Arend Van Spriel wrote:
On November 15, 2023 4:00:46 PM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend van Spriel arend.vanspriel@broadcom.com 于2023年11月13日周一 17:18写道：
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
I'm sure the issue exists because there's only cancler of timer but not woker. As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218, I submit it as soon as I found it.
Ah, yes. The cancel_work_sync() can also be done in brcmf_notify_escan_complete().
AFAIUC, brcmf_notify_scan_complete() is called from the work itself, too, hence you can't issue cancel_work_sync() there (unless you make it conditional).
Takashi
On November 16, 2023 7:25:16 PM Takashi Iwai tiwai@suse.de wrote:
On Thu, 16 Nov 2023 19:20:06 +0100, Arend Van Spriel wrote:
On November 15, 2023 4:00:46 PM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend van Spriel arend.vanspriel@broadcom.com 于2023年11月13日周一 17:18写道：
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
I'm sure the issue exists because there's only cancler of timer but not woker. As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218, I submit it as soon as I found it.
Ah, yes. The cancel_work_sync() can also be done in brcmf_notify_escan_complete().
AFAIUC, brcmf_notify_scan_complete() is called from the work itself, too, hence you can't issue cancel_work_sync() there (unless you make it conditional).
Hi Takashi,
You are obviously right. Let's wait and see what v6 will look like ;-)
Regards, Arend
Yes, that makes this issue hard to fix. I was wondering why it binds the worker with the timer rather than using just one of them.
Takashi Iwai tiwai@suse.de 于2023年11月17日周五 02:25写道：
On Thu, 16 Nov 2023 19:20:06 +0100, Arend Van Spriel wrote:
On November 15, 2023 4:00:46 PM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend van Spriel arend.vanspriel@broadcom.com 于2023年11月13日周一 17:18写道：
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道：
On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
I'm sure the issue exists because there's only cancler of timer but not woker. As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218, I submit it as soon as I found it.
Ah, yes. The cancel_work_sync() can also be done in brcmf_notify_escan_complete().
AFAIUC, brcmf_notify_scan_complete() is called from the work itself, too, hence you can't issue cancel_work_sync() there (unless you make it conditional).
Takashi
On November 17, 2023 3:31:40 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Yes, that makes this issue hard to fix. I was wondering why it binds the worker with the timer rather than using just one of them.
No top posting please!
The timer context is softirq and worker is thread context. The ability to sleep is the big difference between the two or at least the reason for using them here.
Regards, Arend
Takashi Iwai tiwai@suse.de 于2023年11月17日周五 02:25写道：
On Thu, 16 Nov 2023 19:20:06 +0100, Arend Van Spriel wrote:
On November 15, 2023 4:00:46 PM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend van Spriel arend.vanspriel@broadcom.com 于2023年11月13日周一 17:18写道：
On November 8, 2023 4:03:26 AM Zheng Hacker hackerzheng666@gmail.com wrote:
Arend Van Spriel arend.vanspriel@broadcom.com 于2023年11月6日周一 23:48写道： > > On November 6, 2023 3:44:53 PM Zheng Hacker hackerzheng666@gmail.com 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?
Thanks four your detailed review. I did see the code and not sure if brcmf_notify_escan_complete would be triggered for sure. So in the first version I want to delete the pending timer ahead of time.
Why requesting a CVE when you are not sure? Seems a bit hasty to put it mildly.
I'm sure the issue exists because there's only cancler of timer but not woker. As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218, I submit it as soon as I found it.
Ah, yes. The cancel_work_sync() can also be done in brcmf_notify_escan_complete().
AFAIUC, brcmf_notify_scan_complete() is called from the work itself, too, hence you can't issue cancel_work_sync() there (unless you make it conditional).
Takashi
On Mon, 06 Nov 2023, Zheng Wang wrote:
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 zyytlz.wz@163.com Cc: stable@vger.kernel.org
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 there been any progress on this please?
Are we expecting a v6 to this?
linux-stable-mirror@lists.linaro.org
-
Arend Van Spriel -
Arend van Spriel
-
Kalle Valo -
Lee Jones -
Takashi Iwai -
Zheng Hacker -
Zheng Wang