All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cw1200: Revert unnecessary patches that fix unreal use-after-free bugs
@ 2021-05-21 22:32 Hang Zhang
  2021-05-24  1:36 ` Jia-Ju Bai
  2021-06-15 13:41 ` Kalle Valo
  0 siblings, 2 replies; 3+ messages in thread
From: Hang Zhang @ 2021-05-21 22:32 UTC (permalink / raw)
  Cc: Solomon Peachy, Kalle Valo, David S . Miller, Jakub Kicinski,
	Jia-Ju Bai, Wei Yongjun, linux-wireless, netdev, linux-kernel,
	Hang Zhang

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>
---
 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.31.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] cw1200: Revert unnecessary patches that fix unreal use-after-free bugs
  2021-05-21 22:32 [PATCH] cw1200: Revert unnecessary patches that fix unreal use-after-free bugs Hang Zhang
@ 2021-05-24  1:36 ` Jia-Ju Bai
  2021-06-15 13:41 ` Kalle Valo
  1 sibling, 0 replies; 3+ messages in thread
From: Jia-Ju Bai @ 2021-05-24  1:36 UTC (permalink / raw)
  To: Hang Zhang
  Cc: Solomon Peachy, Kalle Valo, David S . Miller, Jakub Kicinski,
	Wei Yongjun, linux-wireless, netdev, linux-kernel

Thanks for fixing my previous mistake.
The patch looks good.

On 2021/5/22 6:32, Hang Zhang wrote:
> 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>
> ---
>   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;
>   }

Acked-by: Jia-Ju Bai <baijiaju1990@gmail.com>


Best wishes,
Jia-Ju Bai

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] cw1200: Revert unnecessary patches that fix unreal use-after-free bugs
  2021-05-21 22:32 [PATCH] cw1200: Revert unnecessary patches that fix unreal use-after-free bugs Hang Zhang
  2021-05-24  1:36 ` Jia-Ju Bai
@ 2021-06-15 13:41 ` Kalle Valo
  1 sibling, 0 replies; 3+ messages in thread
From: Kalle Valo @ 2021-06-15 13:41 UTC (permalink / raw)
  To: Hang Zhang

Hang Zhang <zh.nvgt@gmail.com> wrote:

> 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>

Patch applied to wireless-drivers-next.git, thanks.

3f60f4685699 cw1200: Revert unnecessary patches that fix unreal use-after-free bugs

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210521223238.25020-1-zh.nvgt@gmail.com/

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-15 13:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-21 22:32 [PATCH] cw1200: Revert unnecessary patches that fix unreal use-after-free bugs Hang Zhang
2021-05-24  1:36 ` Jia-Ju Bai
2021-06-15 13:41 ` Kalle Valo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.