| Message ID | 20221212083607.21536-1-quic_wgong@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: change initialize for sk_buff in ieee80211_tx_dequeue() | expand |
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 ? 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) ||
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. 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) ||
On 09.01.23 15: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. Sorry that statement was incomplete: It's only fixed when https://patchwork.kernel.org/project/linux-wireless/patch/20221230121850.218810-1-alexander@wetzel-home.de/ and https://patchwork.kernel.org/project/linux-wireless/patch/20230106223141.98696-1-alexander@wetzel-home.de/ are applied.
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) || >
On 1/9/23 06:22, Alexander Wetzel wrote: > On 09.01.23 15: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. > > Sorry that statement was incomplete: It's only fixed when > https://patchwork.kernel.org/project/linux-wireless/patch/20221230121850.218810-1-alexander@wetzel-home.de/ > and > https://patchwork.kernel.org/project/linux-wireless/patch/20230106223141.98696-1-alexander@wetzel-home.de/ > > are applied. > Ah, yes. That is indeed correct. Let's just hope those patches will apply (and are going to be applied) to stable releases since this is a real problem there which does cause UAF and crashes. Guenter
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) ||
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> --- net/mac80211/tx.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) base-commit: 922932ca02191a390f7f52fb6e21c44b50e14025