[PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[Toke Høiland-Jørgensen]

The TXQ teardown code can reference the vif data structures that are
stored in the netdev private memory area if there are still packets on
the queue when it is being freed. Since the TXQ teardown code is run
after the netdevs are freed, this can lead to a use-after-free. Fix this
by moving the TXQ teardown code to earlier in ieee80211_unregister_hw().

Reported-by: Ben Greear <greearb@candelatech.com>
Tested-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 net/mac80211/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index e054a2fd8d38..98a5c15e8db1 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1170,6 +1170,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 #if IS_ENABLED(CONFIG_IPV6)
 	unregister_inet6addr_notifier(&local->ifa6_notifier);
 #endif
+	ieee80211_txq_teardown_flows(local);
 
 	rtnl_lock();
 
@@ -1198,7 +1199,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	skb_queue_purge(&local->skb_queue);
 	skb_queue_purge(&local->skb_queue_unreliable);
 	skb_queue_purge(&local->skb_queue_tdls_chsw);
-	ieee80211_txq_teardown_flows(local);
 
 	destroy_workqueue(local->workqueue);
 	wiphy_unregister(local->hw.wiphy);
-- 
2.18.0

Re: [PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[Arend van Spriel]

On 8/13/2018 2:16 PM, Toke Høiland-Jørgensen wrote:
> The TXQ teardown code can reference the vif data structures that are
> stored in the netdev private memory area if there are still packets on
> the queue when it is being freed. Since the TXQ teardown code is run
> after the netdevs are freed, this can lead to a use-after-free. Fix this
> by moving the TXQ teardown code to earlier in ieee80211_unregister_hw().

Just off the bat, but from reading the above I am wondering whether the 
use-after-free could also happen upon removing an interface?

Regards,
Arend

> Reported-by: Ben Greear <greearb@candelatech.com>
> Tested-by: Ben Greear <greearb@candelatech.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>   net/mac80211/main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[Ben Greear]

On 08/13/2018 11:25 AM, Arend van Spriel wrote:
> On 8/13/2018 2:16 PM, Toke Høiland-Jørgensen wrote:
>> The TXQ teardown code can reference the vif data structures that are
>> stored in the netdev private memory area if there are still packets on
>> the queue when it is being freed. Since the TXQ teardown code is run
>> after the netdevs are freed, this can lead to a use-after-free. Fix this
>> by moving the TXQ teardown code to earlier in ieee80211_unregister_hw().
>
> Just off the bat, but from reading the above I am wondering whether the use-after-free could also happen upon removing an interface?

At least in practice, it does not seem to happen.  Some of our test cases bring up and down
netdevs very often, and those doe not seem to trigger this bug.

But, could be luck, of course.

Crashing ath10k firmware under tx load, and unloading modules under tx load
seems to be the main trigger.

Thanks,
Ben

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

Re: [PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[Toke Høiland-Jørgensen]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 8/13/2018 2:16 PM, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> The TXQ teardown code can reference the vif data structures that are
>> stored in the netdev private memory area if there are still packets on
>> the queue when it is being freed. Since the TXQ teardown code is run
>> after the netdevs are freed, this can lead to a use-after-free. Fix this
>> by moving the TXQ teardown code to earlier in ieee80211_unregister_hw().
>
> Just off the bat, but from reading the above I am wondering whether
> the use-after-free could also happen upon removing an interface?

Hmm, there doesn't appear to be *any* teardown of TXQs when an interface
is removed...? So I guess that if an interface is removed while it still
has frames on the multicast TXQ, that those packets would be left
hanging there? I don't think there would be an explicit use-after-free,
because they will never get dequeued, so they would just constitute a
memory leak?

Am I missing some automatic mechanism that always empties out queues
before an interface is brought down?

-Toke