| Message ID | 1465563164-783-1-git-send-email-me@bobcopeland.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a66cd733a729167567afd35bca8c3e3a3ace04f6 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 10 June 2016 at 14:52, Bob Copeland <me@bobcopeland.com> wrote: > Smatch warns about a number of cases in ath10k where a pointer is > null-checked after it has already been dereferenced, in code involving > ath10k private virtual interface pointers. > > Fix these by making the dereference happen later. > > Addresses the following smatch warnings: > > drivers/net/wireless/ath/ath10k/mac.c:3651 ath10k_mac_txq_init() warn: variable dereferenced before check 'txq' (see line 3649) > drivers/net/wireless/ath/ath10k/mac.c:3664 ath10k_mac_txq_unref() warn: variable dereferenced before check 'txq' (see line 3659) > drivers/net/wireless/ath/ath10k/htt_tx.c:70 __ath10k_htt_tx_txq_recalc() warn: variable dereferenced before check 'txq->sta' (see line 52) > drivers/net/wireless/ath/ath10k/htt_tx.c:740 ath10k_htt_tx_get_vdev_id() warn: variable dereferenced before check 'cb->vif' (see line 736) > drivers/net/wireless/ath/ath10k/txrx.c:86 ath10k_txrx_tx_unref() warn: variable dereferenced before check 'txq' (see line 84) > drivers/net/wireless/ath/ath10k/wmi.c:1837 ath10k_wmi_op_gen_mgmt_tx() warn: variable dereferenced before check 'cb->vif' (see line 1825) FWIW all of these are false positives. I think this was already pointed out some time ago. The drv_priv stuff is merely an offset (see how ieee80211_vif and ieee80211_sta are defined) and the according structure is always checked beforehand. Michał
On Mon, 2016-06-13 at 07:39 +0200, Michal Kazior wrote: > > FWIW all of these are false positives. I think this was already > pointed out some time ago. The drv_priv stuff is merely an offset > (see how ieee80211_vif and ieee80211_sta are defined) and the > according structure is always checked beforehand. > IIRC, doing something like that can (sometimes?) still get you into undefined behaviour territory, so the compiler could potentially "optimize" away the later NULL check. Or am I confusing something? Seems entirely possible :) johannes
On Mon, Jun 13, 2016 at 11:08:59AM +0200, Johannes Berg wrote: > On Mon, 2016-06-13 at 07:39 +0200, Michal Kazior wrote: > > > > FWIW all of these are false positives. I think this was already > > pointed out some time ago. The drv_priv stuff is merely an offset > > (see how ieee80211_vif and ieee80211_sta are defined) and the > > according structure is always checked beforehand. OK, fair enough, sorry for the noise. I'm daily running sparse / smatch on wireless-testing; although these had been around for a while they showed up as new "errors" because of some line number changes, but I'll squelch them going forward. > IIRC, doing something like that can (sometimes?) still get you into > undefined behaviour territory, so the compiler could potentially > "optimize" away the later NULL check. So I did just go and check the generated code for each of these cases and gcc didn't elide the subsequent if-test, at least on x86-64 and my compiler / build config. Given http://lwn.net/Articles/342330, it seems possible, though.
On Mon, 2016-06-13 at 09:05 -0400, Bob Copeland wrote: > > So I did just go and check the generated code for each of these cases > and gcc didn't elide the subsequent if-test, at least on x86-64 and > my compiler / build config. Given http://lwn.net/Articles/342330, it > seems possible, though. It's not clear that's the same situation, since tun->sk is very likely to have been an actual pointer, not an embedded thing like drv_priv. However, with all this, I think I'd simply not take any chances - the patch isn't exactly invasive and in some cases (for example the first hunk of the patch) will even improve the code to the point where the compiler could warn about uninitialized usage of the pointer when the code gets modified to use it in case of !txq->sta. I'd take it, but I guess it's Kalle's decision :) johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Mon, 2016-06-13 at 09:05 -0400, Bob Copeland wrote: >> >> So I did just go and check the generated code for each of these cases >> and gcc didn't elide the subsequent if-test, at least on x86-64 and >> my compiler / build config. Given http://lwn.net/Articles/342330, it >> seems possible, though. > > It's not clear that's the same situation, since tun->sk is very likely > to have been an actual pointer, not an embedded thing like drv_priv. > > However, with all this, I think I'd simply not take any chances - the > patch isn't exactly invasive and in some cases (for example the first > hunk of the patch) will even improve the code to the point where the > compiler could warn about uninitialized usage of the pointer when the > code gets modified to use it in case of !txq->sta. > > I'd take it, but I guess it's Kalle's decision :) Yeah, I'm leaning towards Johannes. These are not really invasive. -- Kalle Valo
On Tue, Jun 14, 2016 at 01:51:24PM +0000, Kalle Valo wrote: > > It's not clear that's the same situation, since tun->sk is very likely > > to have been an actual pointer, not an embedded thing like drv_priv. Just to follow up on that thread, I did research it a bit yesterday and came to the conclusion that it is UB even when the target is in the same struct. However, in a not very scientific survey, I didn't see either clang or gcc remove the test in a simplified test case (with -O3 and without -fno-delete-null-pointer-checks). If drv_priv were an actual pointer, gcc did remove it but clang did not. So, there's that. > > However, with all this, I think I'd simply not take any chances - the > > patch isn't exactly invasive and in some cases (for example the first > > hunk of the patch) will even improve the code to the point where the > > compiler could warn about uninitialized usage of the pointer when the > > code gets modified to use it in case of !txq->sta. > > > > I'd take it, but I guess it's Kalle's decision :) > > Yeah, I'm leaning towards Johannes. These are not really invasive. Thanks, and sorry about the checkpatch -- I did run checkpatch on it but for some reason my version only complained about some of them.
Bob Copeland <me@bobcopeland.com> writes: >> > However, with all this, I think I'd simply not take any chances - the >> > patch isn't exactly invasive and in some cases (for example the first >> > hunk of the patch) will even improve the code to the point where the >> > compiler could warn about uninitialized usage of the pointer when the >> > code gets modified to use it in case of !txq->sta. >> > >> > I'd take it, but I guess it's Kalle's decision :) >> >> Yeah, I'm leaning towards Johannes. These are not really invasive. > > Thanks, and sorry about the checkpatch -- I did run checkpatch on it but > for some reason my version only complained about some of them. I actually have a custom script which enables (and disables) various checkpatch checks, most likely that's why you didn't see it. https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check
Bob Copeland <me@bobcopeland.com> wrote: > Smatch warns about a number of cases in ath10k where a pointer is > null-checked after it has already been dereferenced, in code involving > ath10k private virtual interface pointers. > > Fix these by making the dereference happen later. > > Addresses the following smatch warnings: > > drivers/net/wireless/ath/ath10k/mac.c:3651 ath10k_mac_txq_init() warn: variable dereferenced before check 'txq' (see line 3649) > drivers/net/wireless/ath/ath10k/mac.c:3664 ath10k_mac_txq_unref() warn: variable dereferenced before check 'txq' (see line 3659) > drivers/net/wireless/ath/ath10k/htt_tx.c:70 __ath10k_htt_tx_txq_recalc() warn: variable dereferenced before check 'txq->sta' (see line 52) > drivers/net/wireless/ath/ath10k/htt_tx.c:740 ath10k_htt_tx_get_vdev_id() warn: variable dereferenced before check 'cb->vif' (see line 736) > drivers/net/wireless/ath/ath10k/txrx.c:86 ath10k_txrx_tx_unref() warn: variable dereferenced before check 'txq' (see line 84) > drivers/net/wireless/ath/ath10k/wmi.c:1837 ath10k_wmi_op_gen_mgmt_tx() warn: variable dereferenced before check 'cb->vif' (see line 1825) > > Signed-off-by: Bob Copeland <me@bobcopeland.com> Thanks, 1 patch applied to ath-next branch of ath.git: a66cd733a729 ath10k: fix potential null dereference bugs
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c index 6269c61..dfcc43d 100644 --- a/drivers/net/wireless/ath/ath10k/htt_tx.c +++ b/drivers/net/wireless/ath/ath10k/htt_tx.c @@ -49,7 +49,7 @@ static void __ath10k_htt_tx_txq_recalc(struct ieee80211_hw *hw, struct ieee80211_txq *txq) { struct ath10k *ar = hw->priv; - struct ath10k_sta *arsta = (void *)txq->sta->drv_priv; + struct ath10k_sta *arsta; struct ath10k_vif *arvif = (void *)txq->vif->drv_priv; unsigned long frame_cnt; unsigned long byte_cnt; @@ -67,10 +67,12 @@ static void __ath10k_htt_tx_txq_recalc(struct ieee80211_hw *hw, if (ar->htt.tx_q_state.mode != HTT_TX_MODE_SWITCH_PUSH_PULL) return; - if (txq->sta) + if (txq->sta) { + arsta = (void *)txq->sta->drv_priv; peer_id = arsta->peer_id; - else + } else { peer_id = arvif->peer_id; + } tid = txq->tid; bit = BIT(peer_id % 32); @@ -733,13 +735,14 @@ static u8 ath10k_htt_tx_get_vdev_id(struct ath10k *ar, struct sk_buff *skb) { struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); struct ath10k_skb_cb *cb = ATH10K_SKB_CB(skb); - struct ath10k_vif *arvif = (void *)cb->vif->drv_priv; + struct ath10k_vif *arvif; if (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN) return ar->scan.vdev_id; - else if (cb->vif) + else if (cb->vif) { + arvif = (void *)cb->vif->drv_priv; return arvif->vdev_id; - else if (ar->monitor_started) + } else if (ar->monitor_started) return ar->monitor_vdev_id; else return 0; diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 6dd1d26..4eee4b9 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -3646,17 +3646,18 @@ void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work) static void ath10k_mac_txq_init(struct ieee80211_txq *txq) { - struct ath10k_txq *artxq = (void *)txq->drv_priv; + struct ath10k_txq *artxq; if (!txq) return; + artxq = (void *)txq->drv_priv; INIT_LIST_HEAD(&artxq->list); } static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq) { - struct ath10k_txq *artxq = (void *)txq->drv_priv; + struct ath10k_txq *artxq; struct ath10k_skb_cb *cb; struct sk_buff *msdu; int msdu_id; @@ -3664,6 +3665,7 @@ static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq) if (!txq) return; + artxq = (void *)txq->drv_priv; spin_lock_bh(&ar->txqs_lock); if (!list_empty(&artxq->list)) list_del_init(&artxq->list); diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c index 576e7c4..9f17863 100644 --- a/drivers/net/wireless/ath/ath10k/txrx.c +++ b/drivers/net/wireless/ath/ath10k/txrx.c @@ -81,10 +81,11 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, skb_cb = ATH10K_SKB_CB(msdu); txq = skb_cb->txq; - artxq = (void *)txq->drv_priv; - if (txq) + if (txq) { + artxq = (void *)txq->drv_priv; artxq->num_fw_queued--; + } ath10k_htt_tx_free_msdu_id(htt, tx_done->msdu_id); ath10k_htt_tx_dec_pending(htt); diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 2c30032..60f78a6 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -1822,7 +1822,7 @@ static struct sk_buff * ath10k_wmi_op_gen_mgmt_tx(struct ath10k *ar, struct sk_buff *msdu) { struct ath10k_skb_cb *cb = ATH10K_SKB_CB(msdu); - struct ath10k_vif *arvif = (void *)cb->vif->drv_priv; + struct ath10k_vif *arvif; struct wmi_mgmt_tx_cmd *cmd; struct ieee80211_hdr *hdr; struct sk_buff *skb; @@ -1834,10 +1834,12 @@ ath10k_wmi_op_gen_mgmt_tx(struct ath10k *ar, struct sk_buff *msdu) hdr = (struct ieee80211_hdr *)msdu->data; fc = le16_to_cpu(hdr->frame_control); - if (cb->vif) + if (cb->vif) { + arvif = (void *)cb->vif->drv_priv; vdev_id = arvif->vdev_id; - else + } else { vdev_id = 0; + } if (WARN_ON_ONCE(!ieee80211_is_mgmt(hdr->frame_control))) return ERR_PTR(-EINVAL);
Smatch warns about a number of cases in ath10k where a pointer is null-checked after it has already been dereferenced, in code involving ath10k private virtual interface pointers. Fix these by making the dereference happen later. Addresses the following smatch warnings: drivers/net/wireless/ath/ath10k/mac.c:3651 ath10k_mac_txq_init() warn: variable dereferenced before check 'txq' (see line 3649) drivers/net/wireless/ath/ath10k/mac.c:3664 ath10k_mac_txq_unref() warn: variable dereferenced before check 'txq' (see line 3659) drivers/net/wireless/ath/ath10k/htt_tx.c:70 __ath10k_htt_tx_txq_recalc() warn: variable dereferenced before check 'txq->sta' (see line 52) drivers/net/wireless/ath/ath10k/htt_tx.c:740 ath10k_htt_tx_get_vdev_id() warn: variable dereferenced before check 'cb->vif' (see line 736) drivers/net/wireless/ath/ath10k/txrx.c:86 ath10k_txrx_tx_unref() warn: variable dereferenced before check 'txq' (see line 84) drivers/net/wireless/ath/ath10k/wmi.c:1837 ath10k_wmi_op_gen_mgmt_tx() warn: variable dereferenced before check 'cb->vif' (see line 1825) Signed-off-by: Bob Copeland <me@bobcopeland.com> --- Although I glanced at a few cases, by and large I didn't check whether the if() tests were correct but assumed they were. Also, I only compile-tested this. drivers/net/wireless/ath/ath10k/htt_tx.c | 15 +++++++++------ drivers/net/wireless/ath/ath10k/mac.c | 6 ++++-- drivers/net/wireless/ath/ath10k/txrx.c | 5 +++-- drivers/net/wireless/ath/ath10k/wmi.c | 8 +++++--- 4 files changed, 21 insertions(+), 13 deletions(-)