[PATCH 1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[PATCH 1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Shuah Khan]

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix ath_tx_process_buffer() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/ath/ath9k/xmit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index e60d4737fc6e..1d36aae3f7b6 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -701,6 +701,9 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 					     ts->ts_rateindex);
 
 	hdr = (struct ieee80211_hdr *) bf->bf_mpdu->data;
+
+	rcu_read_lock();
+
 	sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
 	if (sta) {
 		struct ath_node *an = (struct ath_node *)sta->drv_priv;
@@ -725,6 +728,8 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 
 	if (!flush)
 		ath_txq_schedule(sc, txq);
+
+	rcu_read_unlock();
 }
 
 static bool ath_lookup_legacy(struct ath_buf *bf)
-- 
2.27.0

[PATCH] mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Shuah Khan]

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/mediatek/mt76/mac80211.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index a840396f2c74..3c732da2a53f 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -867,6 +867,9 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	bool ps;
 
 	hw = mt76_phy_hw(dev, status->ext_phy);
+
+	rcu_read_lock();
+
 	if (ieee80211_is_pspoll(hdr->frame_control) && !wcid) {
 		sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, NULL);
 		if (sta)
@@ -876,7 +879,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	mt76_airtime_check(dev, skb);
 
 	if (!wcid || !wcid->sta)
-		return;
+		goto exit;
 
 	sta = container_of((void *)wcid, struct ieee80211_sta, drv_priv);
 
@@ -886,17 +889,17 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	wcid->inactive_count = 0;
 
 	if (!test_bit(MT_WCID_FLAG_CHECK_PS, &wcid->flags))
-		return;
+		goto exit;
 
 	if (ieee80211_is_pspoll(hdr->frame_control)) {
 		ieee80211_sta_pspoll(sta);
-		return;
+		goto exit;
 	}
 
 	if (ieee80211_has_morefrags(hdr->frame_control) ||
 	    !(ieee80211_is_mgmt(hdr->frame_control) ||
 	      ieee80211_is_data(hdr->frame_control)))
-		return;
+		goto exit;
 
 	ps = ieee80211_has_pm(hdr->frame_control);
 
@@ -905,7 +908,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 		ieee80211_sta_uapsd_trigger(sta, status->tid);
 
 	if (!!test_bit(MT_WCID_FLAG_PS, &wcid->flags) == ps)
-		return;
+		goto exit;
 
 	if (ps)
 		set_bit(MT_WCID_FLAG_PS, &wcid->flags);
@@ -914,6 +917,9 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 
 	dev->drv->sta_ps(dev, sta, ps);
 	ieee80211_sta_ps_transition(sta, ps);
+
+exit:
+	rcu_read_unlock();
 }
 
 void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
-- 
2.27.0

Re: [PATCH] mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Felix Fietkau]

On 2021-02-12 03:13, Shuah Khan wrote:
> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix mt76_check_sta() to hold RCU read lock before it calls
> ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only
called from mt76_rx_poll_complete, which itself is only called under RCU
lock.

- Felix

Re: [PATCH] mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Shuah Khan]

On 2/11/21 10:36 PM, Felix Fietkau wrote:
> 
> On 2021-02-12 03:13, Shuah Khan wrote:
>> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
>> the resulting pointer is only valid under RCU lock as well.
>>
>> Fix mt76_check_sta() to hold RCU read lock before it calls
>> ieee80211_find_sta_by_ifaddr() and release it when the resulting
>> pointer is no longer needed.
>>
>> This problem was found while reviewing code to debug RCU warn from
>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>> RCU read lock.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only
> called from mt76_rx_poll_complete, which itself is only called under RCU
> lock.
> 

Yes. You are right. I checked the caller of this routine and didn't
go further up. :)

thanks,
-- Shuah

[PATCH] rtw88: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Shuah Khan]

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/realtek/rtw88/rx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/rx.c b/drivers/net/wireless/realtek/rtw88/rx.c
index 7087e385a9b3..4ab3d3e2bfab 100644
--- a/drivers/net/wireless/realtek/rtw88/rx.c
+++ b/drivers/net/wireless/realtek/rtw88/rx.c
@@ -111,6 +111,9 @@ static void rtw_rx_addr_match_iter(void *data, u8 *mac,
 		return;
 
 	rtw_rx_phy_stat(rtwdev, pkt_stat, hdr);
+
+	rcu_read_lock();
+
 	sta = ieee80211_find_sta_by_ifaddr(rtwdev->hw, hdr->addr2,
 					   vif->addr);
 	if (!sta)
@@ -118,6 +121,9 @@ static void rtw_rx_addr_match_iter(void *data, u8 *mac,
 
 	si = (struct rtw_sta_info *)sta->drv_priv;
 	ewma_rssi_add(&si->avg_rssi, pkt_stat->rssi);
+
+exit:
+	rcu_read_unlock();
 }
 
 static void rtw_rx_addr_match(struct rtw_dev *rtwdev,
-- 
2.27.0

Re: [PATCH] rtw88: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Felix Fietkau]

On 2021-02-12 03:13, Shuah Khan wrote:
> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls
> ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
This one also seems unnecessary. rtw_rx_addr_match_iter is called by
ieee80211_iterate_active_interfaces_atomic, which acquires the RCU read
lock before calling it.

- Felix

Re: [PATCH 1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Felix Fietkau]

On 2021-02-12 03:13, Shuah Khan wrote:
> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix ath_tx_process_buffer() to hold RCU read lock before it calls
> ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
This patch seems unnecessary as well. All callers of
ath_tx_process_buffer seem to hold the RCU read lock already.

- Felix

[PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Shuah Khan]

ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
return pointer (sta) outside null check. Fix it by moving the code
block under the null check.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/ath/ath9k/xmit.c | 28 +++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 1d36aae3f7b6..735858144e3a 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -711,20 +711,24 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 		ath_tx_count_airtime(sc, sta, bf, ts, tid->tidno);
 		if (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY))
 			tid->clear_ps_filter = true;
-	}
 
-	if (!bf_isampdu(bf)) {
-		if (!flush) {
-			info = IEEE80211_SKB_CB(bf->bf_mpdu);
-			memcpy(info->control.rates, bf->rates,
-			       sizeof(info->control.rates));
-			ath_tx_rc_status(sc, bf, ts, 1, txok ? 0 : 1, txok);
-			ath_dynack_sample_tx_ts(sc->sc_ah, bf->bf_mpdu, ts,
-						sta);
+		if (!bf_isampdu(bf)) {
+			if (!flush) {
+				info = IEEE80211_SKB_CB(bf->bf_mpdu);
+				memcpy(info->control.rates, bf->rates,
+				       sizeof(info->control.rates));
+				ath_tx_rc_status(sc, bf, ts, 1,
+						 txok ? 0 : 1, txok);
+				ath_dynack_sample_tx_ts(sc->sc_ah,
+							bf->bf_mpdu, ts, sta);
+			}
+			ath_tx_complete_buf(sc, bf, txq, bf_head, sta,
+					    ts, txok);
+		} else {
+			ath_tx_complete_aggr(sc, txq, bf, bf_head, sta,
+					     tid, ts, txok);
 		}
-		ath_tx_complete_buf(sc, bf, txq, bf_head, sta, ts, txok);
-	} else
-		ath_tx_complete_aggr(sc, txq, bf, bf_head, sta, tid, ts, txok);
+	}
 
 	if (!flush)
 		ath_txq_schedule(sc, txq);
-- 
2.27.0

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Kalle Valo]

Shuah Khan <skhan@linuxfoundation.org> wrote:

> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
> return pointer (sta) outside null check. Fix it by moving the code
> block under the null check.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/43ed9abb9e8d7112f3cc168c2f8c489e253635ba.1613090339.git.skhan@linuxfoundation.org/

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

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Felix Fietkau]

On 2021-02-16 08:03, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> wrote:
> 
>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>> return pointer (sta) outside null check. Fix it by moving the code
>> block under the null check.
>> 
>> This problem was found while reviewing code to debug RCU warn from
>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>> RCU read lock.
>> 
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> 
> Patch applied to ath-next branch of ath.git, thanks.
> 
> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference
I just took another look at this patch, and it is completely bogus.
Not only does the stated reason not make any sense (sta is simply passed
to other functions, not dereferenced without checks), but this also
introduces a horrible memory leak by skipping buffer completion if sta
is NULL.
Please drop it, the code is fine as-is.

- Felix

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Shuah Khan]

On 2/16/21 12:53 AM, Felix Fietkau wrote:
> 
> On 2021-02-16 08:03, Kalle Valo wrote:
>> Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>>> return pointer (sta) outside null check. Fix it by moving the code
>>> block under the null check.
>>>
>>> This problem was found while reviewing code to debug RCU warn from
>>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>>> RCU read lock.
>>>
>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>
>> Patch applied to ath-next branch of ath.git, thanks.
>>
>> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference
> I just took another look at this patch, and it is completely bogus.
> Not only does the stated reason not make any sense (sta is simply passed
> to other functions, not dereferenced without checks), but this also
> introduces a horrible memory leak by skipping buffer completion if sta
> is NULL.
> Please drop it, the code is fine as-is.
> 

A comment describing what you said here might be a good addition to this
comment block though.

thanks,
-- Shuah

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Kalle Valo]

Shuah Khan <skhan@linuxfoundation.org> writes:

> On 2/16/21 12:53 AM, Felix Fietkau wrote:
>>
>> On 2021-02-16 08:03, Kalle Valo wrote:
>>> Shuah Khan <skhan@linuxfoundation.org> wrote:
>>>
>>>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>>>> return pointer (sta) outside null check. Fix it by moving the code
>>>> block under the null check.
>>>>
>>>> This problem was found while reviewing code to debug RCU warn from
>>>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>>>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>>>> RCU read lock.
>>>>
>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>
>>> Patch applied to ath-next branch of ath.git, thanks.
>>>
>>> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference
>> I just took another look at this patch, and it is completely bogus.
>> Not only does the stated reason not make any sense (sta is simply passed
>> to other functions, not dereferenced without checks), but this also
>> introduces a horrible memory leak by skipping buffer completion if sta
>> is NULL.
>> Please drop it, the code is fine as-is.
>
> A comment describing what you said here might be a good addition to this
> comment block though.

Shuah, can you send a followup patch which reverts your change and adds
the comment? I try to avoid rebasing my trees.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Shuah Khan]

On 2/17/21 12:30 AM, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
> 
>> On 2/16/21 12:53 AM, Felix Fietkau wrote:
>>>
>>> On 2021-02-16 08:03, Kalle Valo wrote:
>>>> Shuah Khan <skhan@linuxfoundation.org> wrote:
>>>>
>>>>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>>>>> return pointer (sta) outside null check. Fix it by moving the code
>>>>> block under the null check.
>>>>>
>>>>> This problem was found while reviewing code to debug RCU warn from
>>>>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>>>>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>>>>> RCU read lock.
>>>>>
>>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>>
>>>> Patch applied to ath-next branch of ath.git, thanks.
>>>>
>>>> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference
>>> I just took another look at this patch, and it is completely bogus.
>>> Not only does the stated reason not make any sense (sta is simply passed
>>> to other functions, not dereferenced without checks), but this also
>>> introduces a horrible memory leak by skipping buffer completion if sta
>>> is NULL.
>>> Please drop it, the code is fine as-is.
>>
>> A comment describing what you said here might be a good addition to this
>> comment block though.
> 
> Shuah, can you send a followup patch which reverts your change and adds
> the comment? I try to avoid rebasing my trees.
> 


I can do that.

thanks,
-- Shuah

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Shuah Khan]

On 2/17/21 7:56 AM, Shuah Khan wrote:
> On 2/17/21 12:30 AM, Kalle Valo wrote:
>> Shuah Khan <skhan@linuxfoundation.org> writes:
>>
>>> On 2/16/21 12:53 AM, Felix Fietkau wrote:
>>>>
>>>> On 2021-02-16 08:03, Kalle Valo wrote:
>>>>> Shuah Khan <skhan@linuxfoundation.org> wrote:
>>>>>
>>>>>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>>>>>> return pointer (sta) outside null check. Fix it by moving the code
>>>>>> block under the null check.
>>>>>>
>>>>>> This problem was found while reviewing code to debug RCU warn from
>>>>>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>>>>>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>>>>>> RCU read lock.
>>>>>>
>>>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>>>
>>>>> Patch applied to ath-next branch of ath.git, thanks.
>>>>>
>>>>> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr 
>>>>> dereference
>>>> I just took another look at this patch, and it is completely bogus.
>>>> Not only does the stated reason not make any sense (sta is simply 
>>>> passed
>>>> to other functions, not dereferenced without checks), but this also
>>>> introduces a horrible memory leak by skipping buffer completion if sta
>>>> is NULL.
>>>> Please drop it, the code is fine as-is.
>>>

Felix,

I looked at the code path again and found the following path that
can become a potential dereference downstream. My concern is
about potential dereference downstream.

First path: ath_tx_complete_buf()

1. ath_tx_process_buffer() passes sta to ath_tx_complete_buf()
2. ath_tx_complete_buf() doesn't check or dereference sta
    Passes it on to ath_tx_complete()
3. ath_tx_complete() doesn't check or dereference sta, but assigns
    it to tx_info->status.status_driver_data[0]
    tx_info->status.status_driver_data[0] = sta;

ath_tx_complete_buf() should be fixed to check sta perhaps?

This assignment without checking could lead to dereference at some
point in the future.

Second path: ath_tx_complete_aggr()

1. ath_tx_process_buffer() passes sta to ath_tx_complete_aggr()
2. No problems in this path as ath_tx_complete_aggr() checks
    sta before use.

I can send the revert as it moves more code than necessary under
the null check. As you pointed out, it could lead to memory leak.
Not knowing this code well, I can't really tell where. However,
my original concern is valid for ath_tx_complete_buf() path.

Sending revert as requested.

thanks,
-- Shuah

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Felix Fietkau]

> On 17. Feb 2021, at 21:28, Shuah Khan <skhan@linuxfoundation.org> wrote:
> 
> On 2/17/21 7:56 AM, Shuah Khan wrote:
>>> On 2/17/21 12:30 AM, Kalle Valo wrote:
>>> Shuah Khan <skhan@linuxfoundation.org> writes:
>>> 
>>>> On 2/16/21 12:53 AM, Felix Fietkau wrote:
>>>>> 
>>>>> On 2021-02-16 08:03, Kalle Valo wrote:
>>>>>> Shuah Khan <skhan@linuxfoundation.org> wrote:
>>>>>> 
>>>>>>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>>>>>>> return pointer (sta) outside null check. Fix it by moving the code
>>>>>>> block under the null check.
>>>>>>> 
>>>>>>> This problem was found while reviewing code to debug RCU warn from
>>>>>>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>>>>>>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>>>>>>> RCU read lock.
>>>>>>> 
>>>>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>>>> 
>>>>>> Patch applied to ath-next branch of ath.git, thanks.
>>>>>> 
>>>>>> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference
>>>>> I just took another look at this patch, and it is completely bogus.
>>>>> Not only does the stated reason not make any sense (sta is simply passed
>>>>> to other functions, not dereferenced without checks), but this also
>>>>> introduces a horrible memory leak by skipping buffer completion if sta
>>>>> is NULL.
>>>>> Please drop it, the code is fine as-is.
>>>> 
> 
> Felix,
> 
> I looked at the code path again and found the following path that
> can become a potential dereference downstream. My concern is
> about potential dereference downstream.
> 
> First path: ath_tx_complete_buf()
> 
> 1. ath_tx_process_buffer() passes sta to ath_tx_complete_buf()
> 2. ath_tx_complete_buf() doesn't check or dereference sta
>   Passes it on to ath_tx_complete()
> 3. ath_tx_complete() doesn't check or dereference sta, but assigns
>   it to tx_info->status.status_driver_data[0]
>   tx_info->status.status_driver_data[0] = sta;
> 
> ath_tx_complete_buf() should be fixed to check sta perhaps?
> 
> This assignment without checking could lead to dereference at some
> point in the future.
The assignment is fine, no check needed here. If there was any invalid dereference here, we would see reports of crashes with NULL pointer dereference. Sending packets with sta==NULL is quite common, especially in AP mode.

> Second path: ath_tx_complete_aggr()
> 
> 1. ath_tx_process_buffer() passes sta to ath_tx_complete_aggr()
> 2. No problems in this path as ath_tx_complete_aggr() checks
>   sta before use.
> 
> I can send the revert as it moves more code than necessary under
> the null check. As you pointed out, it could lead to memory leak.
> Not knowing this code well, I can't really tell where. However,
> my original concern is valid for ath_tx_complete_buf
I still don’t see anything to be concerned about. I don’t even think passing a pointer that could be NULL to another function even deserves a comment in the code. Stuff like that is used in many other places as well.

- Felix