[PATCH 1/2] mac80211: validate skb->dev in the tx status path

[PATCH 1/2] mac80211: validate skb->dev in the tx status path

[Felix Fietkau]

skb->dev might contain a stale reference to a device that was already
deleted, and using it unchecked can lead to invalid pointer accesses.
Since this is only used for nl80211 tx, iterate over active interfaces
to find a match for skb->dev, and discard the tx status if the device
is gone.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 net/mac80211/status.c |   46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index b0801b7..6a21562 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -516,30 +516,46 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	}
 
 	if (info->flags & IEEE80211_TX_INTFL_NL80211_FRAME_TX) {
+		struct ieee80211_sub_if_data *p2p_sdata;
 		u64 cookie = (unsigned long)skb;
+		bool found = false;
+
 		acked = info->flags & IEEE80211_TX_STAT_ACK;
 
-		if (ieee80211_is_nullfunc(hdr->frame_control) ||
-		    ieee80211_is_qos_nullfunc(hdr->frame_control)) {
-			cfg80211_probe_status(skb->dev, hdr->addr1,
-					      cookie, acked, GFP_ATOMIC);
-		} else if (skb->dev) {
-			cfg80211_mgmt_tx_status(
-				skb->dev->ieee80211_ptr, cookie, skb->data,
-				skb->len, acked, GFP_ATOMIC);
-		} else {
-			struct ieee80211_sub_if_data *p2p_sdata;
+		rcu_read_lock();
+
+		list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+			if (!sdata->dev)
+				continue;
 
-			rcu_read_lock();
+			if (skb->dev != sdata->dev)
+				continue;
 
+			found = true;
+			break;
+		}
+
+		if (!skb->dev) {
 			p2p_sdata = rcu_dereference(local->p2p_sdata);
 			if (p2p_sdata) {
-				cfg80211_mgmt_tx_status(
-					&p2p_sdata->wdev, cookie, skb->data,
-					skb->len, acked, GFP_ATOMIC);
+				skb->dev = p2p_sdata->dev;
+				found = true;
 			}
-			rcu_read_unlock();
 		}
+
+		if (!found)
+			skb->dev = NULL;
+		else if (ieee80211_is_nullfunc(hdr->frame_control) ||
+			 ieee80211_is_qos_nullfunc(hdr->frame_control)) {
+			cfg80211_probe_status(skb->dev, hdr->addr1,
+					      cookie, acked, GFP_ATOMIC);
+		} else {
+			cfg80211_mgmt_tx_status(
+				skb->dev->ieee80211_ptr, cookie, skb->data,
+				skb->len, acked, GFP_ATOMIC);
+		}
+
+		rcu_read_unlock();
 	}
 
 	if (unlikely(info->ack_frame_id)) {
-- 
1.7.9.6 (Apple Git-31.1)

[PATCH 2/2] mac80211: prevent work queueing while quiescing on suspend

[Felix Fietkau]

Do not emit a warning in that case, since there is nothing else in mac80211
that would effectively prevent more work from being queued.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 net/mac80211/util.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 471fb05..5891741 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -585,6 +585,9 @@ EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces_atomic);
  */
 static bool ieee80211_can_queue_work(struct ieee80211_local *local)
 {
+	if (local->quiescing)
+		return false;
+
 	if (WARN(local->suspended && !local->resuming,
 		 "queueing ieee80211 work while going to suspend\n"))
 		return false;
-- 
1.7.9.6 (Apple Git-31.1)

Re: [PATCH 1/2] mac80211: validate skb->dev in the tx status path

[Johannes Berg]

On Fri, 2012-09-07 at 16:54 +0200, Felix Fietkau wrote:

> +		if (!skb->dev) {
>  			p2p_sdata = rcu_dereference(local->p2p_sdata);
>  			if (p2p_sdata) {
> -				cfg80211_mgmt_tx_status(
> -					&p2p_sdata->wdev, cookie, skb->data,
> -					skb->len, acked, GFP_ATOMIC);
> +				skb->dev = p2p_sdata->dev;
> +				found = true;

What's the point of this? p2p_sdata->dev will be NULL, just like
skb->dev already is?

>  			}
> -			rcu_read_unlock();
>  		}
> +
> +		if (!found)
> +			skb->dev = NULL;
> +		else if (ieee80211_is_nullfunc(hdr->frame_control) ||
> +			 ieee80211_is_qos_nullfunc(hdr->frame_control)) {
> +			cfg80211_probe_status(skb->dev, hdr->addr1,
> +					      cookie, acked, GFP_ATOMIC);
> +		} else {
> +			cfg80211_mgmt_tx_status(
> +				skb->dev->ieee80211_ptr, cookie, skb->data,
> +				skb->len, acked, GFP_ATOMIC);

And therefore this will crash.

johannes

Re: [PATCH 1/2] mac80211: validate skb->dev in the tx status path

[Ben Greear]

On 09/07/2012 07:54 AM, Felix Fietkau wrote:
> skb->dev might contain a stale reference to a device that was already
> deleted, and using it unchecked can lead to invalid pointer accesses.
> Since this is only used for nl80211 tx, iterate over active interfaces
> to find a match for skb->dev, and discard the tx status if the device
> is gone.

Nasty performance hit if we have lots of virtual
interfaces, eh?  Maybe some sort of ref-counting
would be better?  Or a hashed lookup on the netdev
pointer/token?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/2] mac80211: validate skb->dev in the tx status path

[Felix Fietkau]

On 2012-09-07 5:11 PM, Johannes Berg wrote:
> On Fri, 2012-09-07 at 16:54 +0200, Felix Fietkau wrote:
> 
>> +		if (!skb->dev) {
>>  			p2p_sdata = rcu_dereference(local->p2p_sdata);
>>  			if (p2p_sdata) {
>> -				cfg80211_mgmt_tx_status(
>> -					&p2p_sdata->wdev, cookie, skb->data,
>> -					skb->len, acked, GFP_ATOMIC);
>> +				skb->dev = p2p_sdata->dev;
>> +				found = true;
> 
> What's the point of this? p2p_sdata->dev will be NULL, just like
> skb->dev already is?
Oh, missed that. I'll send a v2.

- Felix

Re: [PATCH 1/2] mac80211: validate skb->dev in the tx status path

[Johannes Berg]

On Fri, 2012-09-07 at 08:24 -0700, Ben Greear wrote:
> On 09/07/2012 07:54 AM, Felix Fietkau wrote:
> > skb->dev might contain a stale reference to a device that was already
> > deleted, and using it unchecked can lead to invalid pointer accesses.
> > Since this is only used for nl80211 tx, iterate over active interfaces
> > to find a match for skb->dev, and discard the tx status if the device
> > is gone.
> 
> Nasty performance hit if we have lots of virtual
> interfaces, eh?  Maybe some sort of ref-counting
> would be better?  Or a hashed lookup on the netdev
> pointer/token?

As Felix also pointed out to me, no. Frames that need to go to nl80211
should be rare, this isn't the common case.

johannes

Re: [PATCH 1/2] mac80211: validate skb->dev in the tx status path

[Ben Greear]

On 09/07/2012 08:28 AM, Johannes Berg wrote:
> On Fri, 2012-09-07 at 08:24 -0700, Ben Greear wrote:
>> On 09/07/2012 07:54 AM, Felix Fietkau wrote:
>>> skb->dev might contain a stale reference to a device that was already
>>> deleted, and using it unchecked can lead to invalid pointer accesses.
>>> Since this is only used for nl80211 tx, iterate over active interfaces
>>> to find a match for skb->dev, and discard the tx status if the device
>>> is gone.
>>
>> Nasty performance hit if we have lots of virtual
>> interfaces, eh?  Maybe some sort of ref-counting
>> would be better?  Or a hashed lookup on the netdev
>> pointer/token?
>
> As Felix also pointed out to me, no. Frames that need to go to nl80211
> should be rare, this isn't the common case.

Ahh, ok.  I was thinking each transmitted pkt would cause this code
to run.

Thanks,
Ben

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 2/2] mac80211: prevent work queueing while quiescing on suspend

[Arik Nemtsov]

On Fri, Sep 7, 2012 at 5:54 PM, Felix Fietkau <nbd@openwrt.org> wrote:
> Do not emit a warning in that case, since there is nothing else in mac80211
> that would effectively prevent more work from being queued.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>

This is a bit problematic. You see we rely on this always working
(prior to the suspend handler) to handle the case of HW reconfig
before suspend.

The better option for wlcore would be to flush the workqueue again
after setting suspended = true, but i'm not sure if it's possible.

At the very least we'll have to know the work was not queued (via a ret val?).

Re: [PATCH 2/2] mac80211: prevent work queueing while quiescing on suspend

[Arik Nemtsov]

On Sun, Sep 9, 2012 at 12:15 PM, Arik Nemtsov <arik@wizery.com> wrote:
> On Fri, Sep 7, 2012 at 5:54 PM, Felix Fietkau <nbd@openwrt.org> wrote:
>> Do not emit a warning in that case, since there is nothing else in mac80211
>> that would effectively prevent more work from being queued.
>>
>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>
> This is a bit problematic. You see we rely on this always working
> (prior to the suspend handler) to handle the case of HW reconfig
> before suspend.
>
> The better option for wlcore would be to flush the workqueue again
> after setting suspended = true, but i'm not sure if it's possible.
>
> At the very least we'll have to know the work was not queued (via a ret val?).

Actually this is problematic in general, as lower level driver use
ieee80211_queue_work for day to day stuff, and are not aware of
mac80211 state (was pointed out by Eliad).

While it makes sense to expect a driver to not queue work after its
suspend handler is called, you're doing something quite different
here.
So adding a retval is not good enough, as it's a big API change for
the drivers, that suddenly have to check every queue_work call.

Maybe turning this into a freezable wq is a good idea? Maybe just fail
works queued from within mac80211? (but that can be done from inside
by just checking the quiescing flag inside the work)