| Message ID | 43ed9abb9e8d7112f3cc168c2f8c489e253635ba.1613090339.git.skhan@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a56c14bb21b296fb6d395164ab62ef2e419e5069 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr() | expand |
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
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
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
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.
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
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
> 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
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);
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(-)