| From 8fb8afcd3930e0f4c8e3df8d79eb2b9df7119c2e Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Fri, 21 May 2021 15:32:38 -0700 |
| Subject: cw1200: Revert unnecessary patches that fix unreal use-after-free |
| bugs |
| |
| From: Hang Zhang <zh.nvgt@gmail.com> |
| |
| [ Upstream commit 3f60f4685699aa6006e58e424637e8e413e0a94d ] |
| |
| A previous commit 4f68ef64cd7f ("cw1200: Fix concurrency |
| use-after-free bugs in cw1200_hw_scan()") tried to fix a seemingly |
| use-after-free bug between cw1200_bss_info_changed() and |
| cw1200_hw_scan(), where the former frees a sk_buff pointed |
| to by frame.skb, and the latter accesses the sk_buff |
| pointed to by frame.skb. However, this issue should be a |
| false alarm because: |
| |
| (1) "frame.skb" is not a shared variable between the above |
| two functions, because "frame" is a local function variable, |
| each of the two functions has its own local "frame" - they |
| just happen to have the same variable name. |
| |
| (2) the sk_buff(s) pointed to by these two "frame.skb" are |
| also two different object instances, they are individually |
| allocated by different dev_alloc_skb() within the two above |
| functions. To free one object instance will not invalidate |
| the access of another different one. |
| |
| Based on these facts, the previous commit should be unnecessary. |
| Moreover, it also introduced a missing unlock which was |
| addressed in a subsequent commit 51c8d24101c7 ("cw1200: fix missing |
| unlock on error in cw1200_hw_scan()"). Now that the |
| original use-after-free is unreal, these two commits should |
| be reverted. This patch performs the reversion. |
| |
| Fixes: 4f68ef64cd7f ("cw1200: Fix concurrency use-after-free bugs in cw1200_hw_scan()") |
| Fixes: 51c8d24101c7 ("cw1200: fix missing unlock on error in cw1200_hw_scan()") |
| Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> |
| Acked-by: Jia-Ju Bai <baijiaju1990@gmail.com> |
| Signed-off-by: Kalle Valo <kvalo@codeaurora.org> |
| Link: https://lore.kernel.org/r/20210521223238.25020-1-zh.nvgt@gmail.com |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/net/wireless/st/cw1200/scan.c | 17 +++++++---------- |
| 1 file changed, 7 insertions(+), 10 deletions(-) |
| |
| diff --git a/drivers/net/wireless/st/cw1200/scan.c b/drivers/net/wireless/st/cw1200/scan.c |
| index 988581cc134b..1f856fbbc0ea 100644 |
| --- a/drivers/net/wireless/st/cw1200/scan.c |
| +++ b/drivers/net/wireless/st/cw1200/scan.c |
| @@ -75,30 +75,27 @@ int cw1200_hw_scan(struct ieee80211_hw *hw, |
| if (req->n_ssids > WSM_SCAN_MAX_NUM_OF_SSIDS) |
| return -EINVAL; |
| |
| - /* will be unlocked in cw1200_scan_work() */ |
| - down(&priv->scan.lock); |
| - mutex_lock(&priv->conf_mutex); |
| - |
| frame.skb = ieee80211_probereq_get(hw, priv->vif->addr, NULL, 0, |
| req->ie_len); |
| - if (!frame.skb) { |
| - mutex_unlock(&priv->conf_mutex); |
| - up(&priv->scan.lock); |
| + if (!frame.skb) |
| return -ENOMEM; |
| - } |
| |
| if (req->ie_len) |
| skb_put_data(frame.skb, req->ie, req->ie_len); |
| |
| + /* will be unlocked in cw1200_scan_work() */ |
| + down(&priv->scan.lock); |
| + mutex_lock(&priv->conf_mutex); |
| + |
| ret = wsm_set_template_frame(priv, &frame); |
| if (!ret) { |
| /* Host want to be the probe responder. */ |
| ret = wsm_set_probe_responder(priv, true); |
| } |
| if (ret) { |
| - dev_kfree_skb(frame.skb); |
| mutex_unlock(&priv->conf_mutex); |
| up(&priv->scan.lock); |
| + dev_kfree_skb(frame.skb); |
| return ret; |
| } |
| |
| @@ -120,8 +117,8 @@ int cw1200_hw_scan(struct ieee80211_hw *hw, |
| ++priv->scan.n_ssids; |
| } |
| |
| - dev_kfree_skb(frame.skb); |
| mutex_unlock(&priv->conf_mutex); |
| + dev_kfree_skb(frame.skb); |
| queue_work(priv->workqueue, &priv->scan.work); |
| return 0; |
| } |
| -- |
| 2.30.2 |
| |