From: Guenter Roeck <linux@roeck-us.net> To: Alexander Wetzel <alexander@wetzel-home.de>, Wen Gong <quic_wgong@quicinc.com> Cc: johannes@sipsolutions.net, ath11k@lists.infradead.org, linux-wireless@vger.kernel.org Subject: Re: [PATCH] wifi: mac80211: change initialize for sk_buff in ieee80211_tx_dequeue() Date: Mon, 9 Jan 2023 07:39:34 -0800 [thread overview] Message-ID: <6b39574f-e067-395f-f579-b0b805983996@roeck-us.net> (raw) In-Reply-To: <d28c6340-7ad2-d7f6-939d-3a1a057cb7da@wetzel-home.de> On 1/9/23 06:05, Alexander Wetzel wrote: > On 05.01.23 06:26, Guenter Roeck wrote: >> On Mon, Dec 12, 2022 at 03:36:07AM -0500, Wen Gong wrote: >>> The sk_buff is only set to NULL when initialize, sometimes it will goto >>> label "begin" after ieee80211_free_txskb(), then it points to a sk_buff >>> which is already freed. If it run into the "goto out" after arrived to >>> label "begin", then it will return a sk_buff which is freed, it is a >>> risk for use-after-free. >>> >>> Fixes: ded4698b58cb ("mac80211: run late dequeue late tx handlers without holding fq->lock") >>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com> >> >> I don't see any progress on this patch. Is there a problem with it ? >> Did it get lost ? >> > > Looks ok for me. But I just noticed that my patch > https://patchwork.kernel.org/project/linux-wireless/patch/20221230121850.218810-1-alexander@wetzel-home.de/ > > should also fix the issue as an unintended side effect. > Unless I am missing something, there is still a path begin: ... if (unlikely(test_bit(IEEE80211_TXQ_STOP, &txqi->flags))) goto out; ... skb = ... ... ieee80211_free_txskb(&local->hw, skb); goto begin; after your patch is applied. Unless the IEEE80211_TXQ_STOP can never be true after the first iteration I don't see how your patch would fix the problem. Guenter > Alexander > >> Thanks, >> Guenter >> >>> --- >>> net/mac80211/tx.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> >>> base-commit: 922932ca02191a390f7f52fb6e21c44b50e14025 >>> >>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c >>> index 2171cd1ca807..0b23cc9ab9c7 100644 >>> --- a/net/mac80211/tx.c >>> +++ b/net/mac80211/tx.c >>> @@ -3776,7 +3776,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, >>> struct ieee80211_local *local = hw_to_local(hw); >>> struct txq_info *txqi = container_of(txq, struct txq_info, txq); >>> struct ieee80211_hdr *hdr; >>> - struct sk_buff *skb = NULL; >>> + struct sk_buff *skb; >>> struct fq *fq = &local->fq; >>> struct fq_tin *tin = &txqi->tin; >>> struct ieee80211_tx_info *info; >>> @@ -3790,6 +3790,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, >>> return NULL; >>> begin: >>> + skb = NULL; >>> + >>> spin_lock_bh(&fq->lock); >>> if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || > -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k
WARNING: multiple messages have this Message-ID (diff)
From: Guenter Roeck <linux@roeck-us.net> To: Alexander Wetzel <alexander@wetzel-home.de>, Wen Gong <quic_wgong@quicinc.com> Cc: johannes@sipsolutions.net, ath11k@lists.infradead.org, linux-wireless@vger.kernel.org Subject: Re: [PATCH] wifi: mac80211: change initialize for sk_buff in ieee80211_tx_dequeue() Date: Mon, 9 Jan 2023 07:39:34 -0800 [thread overview] Message-ID: <6b39574f-e067-395f-f579-b0b805983996@roeck-us.net> (raw) In-Reply-To: <d28c6340-7ad2-d7f6-939d-3a1a057cb7da@wetzel-home.de> On 1/9/23 06:05, Alexander Wetzel wrote: > On 05.01.23 06:26, Guenter Roeck wrote: >> On Mon, Dec 12, 2022 at 03:36:07AM -0500, Wen Gong wrote: >>> The sk_buff is only set to NULL when initialize, sometimes it will goto >>> label "begin" after ieee80211_free_txskb(), then it points to a sk_buff >>> which is already freed. If it run into the "goto out" after arrived to >>> label "begin", then it will return a sk_buff which is freed, it is a >>> risk for use-after-free. >>> >>> Fixes: ded4698b58cb ("mac80211: run late dequeue late tx handlers without holding fq->lock") >>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com> >> >> I don't see any progress on this patch. Is there a problem with it ? >> Did it get lost ? >> > > Looks ok for me. But I just noticed that my patch > https://patchwork.kernel.org/project/linux-wireless/patch/20221230121850.218810-1-alexander@wetzel-home.de/ > > should also fix the issue as an unintended side effect. > Unless I am missing something, there is still a path begin: ... if (unlikely(test_bit(IEEE80211_TXQ_STOP, &txqi->flags))) goto out; ... skb = ... ... ieee80211_free_txskb(&local->hw, skb); goto begin; after your patch is applied. Unless the IEEE80211_TXQ_STOP can never be true after the first iteration I don't see how your patch would fix the problem. Guenter > Alexander > >> Thanks, >> Guenter >> >>> --- >>> net/mac80211/tx.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> >>> base-commit: 922932ca02191a390f7f52fb6e21c44b50e14025 >>> >>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c >>> index 2171cd1ca807..0b23cc9ab9c7 100644 >>> --- a/net/mac80211/tx.c >>> +++ b/net/mac80211/tx.c >>> @@ -3776,7 +3776,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, >>> struct ieee80211_local *local = hw_to_local(hw); >>> struct txq_info *txqi = container_of(txq, struct txq_info, txq); >>> struct ieee80211_hdr *hdr; >>> - struct sk_buff *skb = NULL; >>> + struct sk_buff *skb; >>> struct fq *fq = &local->fq; >>> struct fq_tin *tin = &txqi->tin; >>> struct ieee80211_tx_info *info; >>> @@ -3790,6 +3790,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, >>> return NULL; >>> begin: >>> + skb = NULL; >>> + >>> spin_lock_bh(&fq->lock); >>> if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || >
next prev parent reply other threads:[~2023-01-09 15:41 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-12 8:36 [PATCH] wifi: mac80211: change initialize for sk_buff in ieee80211_tx_dequeue() Wen Gong 2022-12-12 8:36 ` Wen Gong 2023-01-05 5:26 ` Guenter Roeck 2023-01-05 5:26 ` Guenter Roeck 2023-01-09 14:05 ` Alexander Wetzel 2023-01-09 14:05 ` Alexander Wetzel 2023-01-09 14:22 ` Alexander Wetzel 2023-01-09 14:22 ` Alexander Wetzel 2023-01-09 15:43 ` Guenter Roeck 2023-01-09 15:43 ` Guenter Roeck 2023-01-09 15:39 ` Guenter Roeck [this message] 2023-01-09 15:39 ` Guenter Roeck
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6b39574f-e067-395f-f579-b0b805983996@roeck-us.net \ --to=linux@roeck-us.net \ --cc=alexander@wetzel-home.de \ --cc=ath11k@lists.infradead.org \ --cc=johannes@sipsolutions.net \ --cc=linux-wireless@vger.kernel.org \ --cc=quic_wgong@quicinc.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.