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 <[email protected]>
---
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(-)
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);
--
2.1.4
Sm9oYW5uZXMgQmVyZyA8am9oYW5uZXNAc2lwc29sdXRpb25zLm5ldD4gd3JpdGVzOg0KDQo+IE9u
IE1vbiwgMjAxNi0wNi0xMyBhdCAwOTowNSAtMDQwMCwgQm9iIENvcGVsYW5kIHdyb3RlOg0KPj7C
oA0KPj4gU28gSSBkaWQganVzdCBnbyBhbmQgY2hlY2sgdGhlIGdlbmVyYXRlZCBjb2RlIGZvciBl
YWNoIG9mIHRoZXNlIGNhc2VzDQo+PiBhbmQgZ2NjIGRpZG4ndCBlbGlkZSB0aGUgc3Vic2VxdWVu
dCBpZi10ZXN0LCBhdCBsZWFzdCBvbiB4ODYtNjQgYW5kDQo+PiBteSBjb21waWxlciAvIGJ1aWxk
IGNvbmZpZy7CoMKgR2l2ZW4gaHR0cDovL2x3bi5uZXQvQXJ0aWNsZXMvMzQyMzMwLCBpdA0KPj4g
c2VlbXMgcG9zc2libGUsIHRob3VnaC4NCj4NCj4gSXQncyBub3QgY2xlYXIgdGhhdCdzIHRoZSBz
YW1lIHNpdHVhdGlvbiwgc2luY2UgdHVuLT5zayBpcyB2ZXJ5IGxpa2VseQ0KPiB0byBoYXZlIGJl
ZW4gYW4gYWN0dWFsIHBvaW50ZXIsIG5vdCBhbiBlbWJlZGRlZCB0aGluZyBsaWtlIGRydl9wcml2
Lg0KPg0KPiBIb3dldmVyLCB3aXRoIGFsbCB0aGlzLCBJIHRoaW5rIEknZCBzaW1wbHkgbm90IHRh
a2UgYW55IGNoYW5jZXMgLSB0aGUNCj4gcGF0Y2ggaXNuJ3QgZXhhY3RseSBpbnZhc2l2ZSBhbmQg
aW4gc29tZSBjYXNlcyAoZm9yIGV4YW1wbGUgdGhlIGZpcnN0DQo+IGh1bmsgb2YgdGhlIHBhdGNo
KSB3aWxsIGV2ZW4gaW1wcm92ZSB0aGUgY29kZSB0byB0aGUgcG9pbnQgd2hlcmUgdGhlDQo+IGNv
bXBpbGVyIGNvdWxkIHdhcm4gYWJvdXQgdW5pbml0aWFsaXplZCB1c2FnZSBvZiB0aGUgcG9pbnRl
ciB3aGVuIHRoZQ0KPiBjb2RlIGdldHMgbW9kaWZpZWQgdG8gdXNlIGl0IGluIGNhc2Ugb2YgIXR4
cS0+c3RhLg0KPg0KPiBJJ2QgdGFrZSBpdCwgYnV0IEkgZ3Vlc3MgaXQncyBLYWxsZSdzIGRlY2lz
aW9uIDopDQoNClllYWgsIEknbSBsZWFuaW5nIHRvd2FyZHMgSm9oYW5uZXMuIFRoZXNlIGFyZSBu
b3QgcmVhbGx5IGludmFzaXZlLg0KDQotLSANCkthbGxlIFZhbG8=
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
Bob Copeland <[email protected]> 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
--
Kalle Valo
Bob Copeland <[email protected]> writes:
> 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 <[email protected]>
There was a new checkpatch warning:
drivers/net/wireless/ath/ath10k/htt_tx.c:740: braces {} should be used on all arms of this statement
I "fixed" it like this, which is folded to the patch in pending branch
(pushed soon):
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index dfcc43d80808..ae5b33fe5ba8 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -737,15 +737,16 @@ static u8 ath10k_htt_tx_get_vdev_id(struct ath10k *ar, struct sk_buff *skb)
struct ath10k_skb_cb *cb = ATH10K_SKB_CB(skb);
struct ath10k_vif *arvif;
- if (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN)
+ 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
+ } else {
return 0;
+ }
}
static u8 ath10k_htt_tx_get_tid(struct sk_buff *skb, bool is_eth)
Bob Copeland <[email protected]> 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 <[email protected]>
Thanks, 1 patch applied to ath-next branch of ath.git:
a66cd733a729 ath10k: fix potential null dereference bugs
--
Sent by pwcli
https://patchwork.kernel.org/patch/9169669/
On 10 June 2016 at 14:52, Bob Copeland <[email protected]> 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 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
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 %% http://bobcopeland.com/
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.
--
Bob Copeland %% http://bobcopeland.com/