[PATCH 0/2] Fixes packet processes after vif is stopped

[PATCH 0/2] Fixes packet processes after vif is stopped

[Remi Pommarel]

Those are a couple of fixes that prevent crashes due to processing
packets (especially multicast ones) for TX after vif is stopped (either
after a mesh interface left the group or interface is put down).

The first one ensure the key info passed to drivers through ieee80211
skb control block is up to date, even after key removal.

The second one ensure no packets get processed after vif driver private
data is cleared in ieee80211_do_stop().

As I tried to explain in second patch footnote, I can still see a
theoretical reason that packets get queued after ieee80211_do_stop()
call. But I was not able to reproduce it, so I may be missing a
something here; making that more as an open question.

Remi Pommarel (2):
  wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()
  wifi: mac80211: Purge vif txq in ieee80211_do_stop()

 net/mac80211/iface.c | 3 +++
 net/mac80211/tx.c    | 6 ++++++
 2 files changed, 9 insertions(+)

-- 
2.40.0

[PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Remi Pommarel]

The ieee80211 skb control block key (set when skb was queued) could have
been removed before ieee80211_tx_dequeue() call. ieee80211_tx_dequeue()
already called ieee80211_tx_h_select_key() to get the current key, but
the latter do not update the key in skb control block in case it is
NULL. Because some drivers actually use this key in their TX callbacks
(e.g. ath1{1,2}k_mac_op_tx()) this could lead to the use after free
below:

  BUG: KASAN: slab-use-after-free in ath11k_mac_op_tx+0x590/0x61c
  Read of size 4 at addr ffffff803083c248 by task kworker/u16:4/1440

  CPU: 3 UID: 0 PID: 1440 Comm: kworker/u16:4 Not tainted 6.13.0-ge128f627f404 #2
  Hardware name: HW (DT)
  Workqueue: bat_events batadv_send_outstanding_bcast_packet
  Call trace:
   show_stack+0x14/0x1c (C)
   dump_stack_lvl+0x58/0x74
   print_report+0x164/0x4c0
   kasan_report+0xac/0xe8
   __asan_report_load4_noabort+0x1c/0x24
   ath11k_mac_op_tx+0x590/0x61c
   ieee80211_handle_wake_tx_queue+0x12c/0x1c8
   ieee80211_queue_skb+0xdcc/0x1b4c
   ieee80211_tx+0x1ec/0x2bc
   ieee80211_xmit+0x224/0x324
   __ieee80211_subif_start_xmit+0x85c/0xcf8
   ieee80211_subif_start_xmit+0xc0/0xec4
   dev_hard_start_xmit+0xf4/0x28c
   __dev_queue_xmit+0x6ac/0x318c
   batadv_send_skb_packet+0x38c/0x4b0
   batadv_send_outstanding_bcast_packet+0x110/0x328
   process_one_work+0x578/0xc10
   worker_thread+0x4bc/0xc7c
   kthread+0x2f8/0x380
   ret_from_fork+0x10/0x20

  Allocated by task 1906:
   kasan_save_stack+0x28/0x4c
   kasan_save_track+0x1c/0x40
   kasan_save_alloc_info+0x3c/0x4c
   __kasan_kmalloc+0xac/0xb0
   __kmalloc_noprof+0x1b4/0x380
   ieee80211_key_alloc+0x3c/0xb64
   ieee80211_add_key+0x1b4/0x71c
   nl80211_new_key+0x2b4/0x5d8
   genl_family_rcv_msg_doit+0x198/0x240
  <...>

  Freed by task 1494:
   kasan_save_stack+0x28/0x4c
   kasan_save_track+0x1c/0x40
   kasan_save_free_info+0x48/0x94
   __kasan_slab_free+0x48/0x60
   kfree+0xc8/0x31c
   kfree_sensitive+0x70/0x80
   ieee80211_key_free_common+0x10c/0x174
   ieee80211_free_keys+0x188/0x46c
   ieee80211_stop_mesh+0x70/0x2cc
   ieee80211_leave_mesh+0x1c/0x60
   cfg80211_leave_mesh+0xe0/0x280
   cfg80211_leave+0x1e0/0x244
  <...>

Update SKB control block key even when key is NULL to avoid that.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 net/mac80211/tx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index a24636bda679..79c217c2f801 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -668,6 +668,12 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
 		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
 		return TX_DROP;
+	} else {
+		/* Clear SKB CB key reference, ieee80211_tx_h_select_key()
+		 * could have been called to update key info after its removal
+		 * (e.g. by ieee80211_tx_dequeue()).
+		 */
+		info->control.hw_key = NULL;
 	}
 
 	return TX_CONTINUE;
-- 
2.40.0

Re: [PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Johannes Berg]

On Fri, 2025-03-14 at 12:04 +0100, Remi Pommarel wrote:
> The ieee80211 skb control block key (set when skb was queued) could have
> been removed before ieee80211_tx_dequeue() call. ieee80211_tx_dequeue()
> already called ieee80211_tx_h_select_key() to get the current key, but
> the latter do not update the key in skb control block in case it is
> NULL. Because some drivers actually use this key in their TX callbacks
> (e.g. ath1{1,2}k_mac_op_tx()) this could lead to the use after free
> below:
> 
>   BUG: KASAN: slab-use-after-free in ath11k_mac_op_tx+0x590/0x61c
>   Read of size 4 at addr ffffff803083c248 by task kworker/u16:4/1440


Maybe should have a Fixes: tag?

And please also tag the subject "[PATCH wireless NN/MM]".

> +++ b/net/mac80211/tx.c
> @@ -668,6 +668,12 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
>  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
>  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
>  		return TX_DROP;
> +	} else {
> +		/* Clear SKB CB key reference, ieee80211_tx_h_select_key()
> +		 * could have been called to update key info after its removal
> +		 * (e.g. by ieee80211_tx_dequeue()).
> +		 */
> +		info->control.hw_key = NULL;
>  	}

I'm not sure this looks like the right place - should probably be done
around line 3897 before the call:

        /*
         * The key can be removed while the packet was queued, so need to call
         * this here to get the current key.
         */
        r = ieee80211_tx_h_select_key(&tx);


I'd think?

johannes

Re: [PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Remi Pommarel]

On Mon, Mar 24, 2025 at 01:17:08PM +0100, Johannes Berg wrote:
> On Fri, 2025-03-14 at 12:04 +0100, Remi Pommarel wrote:
> > The ieee80211 skb control block key (set when skb was queued) could have
> > been removed before ieee80211_tx_dequeue() call. ieee80211_tx_dequeue()
> > already called ieee80211_tx_h_select_key() to get the current key, but
> > the latter do not update the key in skb control block in case it is
> > NULL. Because some drivers actually use this key in their TX callbacks
> > (e.g. ath1{1,2}k_mac_op_tx()) this could lead to the use after free
> > below:
> > 
> >   BUG: KASAN: slab-use-after-free in ath11k_mac_op_tx+0x590/0x61c
> >   Read of size 4 at addr ffffff803083c248 by task kworker/u16:4/1440
> 
> 
> Maybe should have a Fixes: tag?

Finding a fix tag is not easy for this case because I am not sure which
commit exactly introduced the issue. Is it the introduction of
ieee80211_handle_wake_tx_queue() (i.e. c850e31f79f0) that allows packets
queued on another dev to be processed or the one that introduced
ieee80211_tx_dequeue() (i.e.  bb42f2d13ffc) ?

I would have said the latter, what do you think ?

> 
> And please also tag the subject "[PATCH wireless NN/MM]".

Sure I have seen the new subject tag discussion too late unfortunately.

> 
> > +++ b/net/mac80211/tx.c
> > @@ -668,6 +668,12 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> >  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
> >  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> >  		return TX_DROP;
> > +	} else {
> > +		/* Clear SKB CB key reference, ieee80211_tx_h_select_key()
> > +		 * could have been called to update key info after its removal
> > +		 * (e.g. by ieee80211_tx_dequeue()).
> > +		 */
> > +		info->control.hw_key = NULL;
> >  	}
> 
> I'm not sure this looks like the right place - should probably be done
> around line 3897 before the call:
> 
>         /*
>          * The key can be removed while the packet was queued, so need to call
>          * this here to get the current key.
>          */
>         r = ieee80211_tx_h_select_key(&tx);
> 
> 
> I'd think?

I initially did that, but because I ended up with the following:

+	info.control.hw_key = (tx->key) ? &tx->key.conf: NULL;

I found it more readable to do that directly in
ieee80211_tx_h_select_key(). But I don't have strong feeling about that.
So both ways are fine with me, let me know which one like the most ?

-- 
Remi

Re: [PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Remi Pommarel]

On Mon, Mar 24, 2025 at 03:02:48PM +0100, Remi Pommarel wrote:
> On Mon, Mar 24, 2025 at 01:17:08PM +0100, Johannes Berg wrote:
> > On Fri, 2025-03-14 at 12:04 +0100, Remi Pommarel wrote:
> > > The ieee80211 skb control block key (set when skb was queued) could have
> > > been removed before ieee80211_tx_dequeue() call. ieee80211_tx_dequeue()
> > > already called ieee80211_tx_h_select_key() to get the current key, but
> > > the latter do not update the key in skb control block in case it is
> > > NULL. Because some drivers actually use this key in their TX callbacks
> > > (e.g. ath1{1,2}k_mac_op_tx()) this could lead to the use after free
> > > below:
> > > 
> > >   BUG: KASAN: slab-use-after-free in ath11k_mac_op_tx+0x590/0x61c
> > >   Read of size 4 at addr ffffff803083c248 by task kworker/u16:4/1440
> > 
> > 
> > Maybe should have a Fixes: tag?
> 
> Finding a fix tag is not easy for this case because I am not sure which
> commit exactly introduced the issue. Is it the introduction of
> ieee80211_handle_wake_tx_queue() (i.e. c850e31f79f0) that allows packets
> queued on another dev to be processed or the one that introduced
> ieee80211_tx_dequeue() (i.e.  bb42f2d13ffc) ?
> 
> I would have said the latter, what do you think ?
> 
> > 
> > And please also tag the subject "[PATCH wireless NN/MM]".
> 
> Sure I have seen the new subject tag discussion too late unfortunately.
> 
> > 
> > > +++ b/net/mac80211/tx.c
> > > @@ -668,6 +668,12 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
> > >  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> > >  		return TX_DROP;
> > > +	} else {
> > > +		/* Clear SKB CB key reference, ieee80211_tx_h_select_key()
> > > +		 * could have been called to update key info after its removal
> > > +		 * (e.g. by ieee80211_tx_dequeue()).
> > > +		 */
> > > +		info->control.hw_key = NULL;
> > >  	}
> > 
> > I'm not sure this looks like the right place - should probably be done
> > around line 3897 before the call:
> > 
> >         /*
> >          * The key can be removed while the packet was queued, so need to call
> >          * this here to get the current key.
> >          */
> >         r = ieee80211_tx_h_select_key(&tx);
> > 
> > 
> > I'd think?
> 
> I initially did that, but because I ended up with the following:
> 
> +	info.control.hw_key = (tx->key) ? &tx->key.conf: NULL;
> 
> I found it more readable to do that directly in
> ieee80211_tx_h_select_key(). But I don't have strong feeling about that.
> So both ways are fine with me, let me know which one like the most ?

Oh sorry, you meant to initialize to NULL *before* the call to
ieee80211_tx_h_select_key(), sure will do that instead.

-- 
Remi

Re: [PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Johannes Berg]

On Mon, 2025-03-24 at 15:02 +0100, Remi Pommarel wrote:
> 
> Finding a fix tag is not easy for this case because I am not sure which
> commit exactly introduced the issue. Is it the introduction of
> ieee80211_handle_wake_tx_queue() (i.e. c850e31f79f0) that allows packets
> queued on another dev to be processed or the one that introduced
> ieee80211_tx_dequeue() (i.e.  bb42f2d13ffc) ?
> 
> I would have said the latter, what do you think ?

I would agree, despite saying "Move ..." that called
ieee80211_tx_h_select_key() twice, once in invoke_tx_handlers_early()
before queueing the frame, and again in ieee80211_tx_dequeue().

johannes

Re: [PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2025-03-24 at 15:02 +0100, Remi Pommarel wrote:
>> 
>> Finding a fix tag is not easy for this case because I am not sure which
>> commit exactly introduced the issue. Is it the introduction of
>> ieee80211_handle_wake_tx_queue() (i.e. c850e31f79f0) that allows packets
>> queued on another dev to be processed or the one that introduced
>> ieee80211_tx_dequeue() (i.e.  bb42f2d13ffc) ?
>> 
>> I would have said the latter, what do you think ?
>
> I would agree, despite saying "Move ..." that called
> ieee80211_tx_h_select_key() twice, once in invoke_tx_handlers_early()
> before queueing the frame, and again in ieee80211_tx_dequeue().

Yeah, agreed :)

-Toke

[PATCH 2/2] wifi: mac80211: Purge vif txq in ieee80211_do_stop()

[Remi Pommarel]

After ieee80211_do_stop() SKB from vif's txq could still be processed.
Indeed another concurrent vif schedule_and_wake_txq call could cause
those packets to be dequeued (see ieee80211_handle_wake_tx_queue())
without checking the sdata current state.

Because vif.drv_priv is now cleared in this function, this could lead to
driver crash.

For example in ath12k, ahvif is store in vif.drv_priv. Thus if
ath12k_mac_op_tx() is called after ieee80211_do_stop(), ahvif->ah can be
NULL, leading the ath12k_warn(ahvif->ah,...) call in this function to
trigger the NULL deref below.

  Unable to handle kernel paging request at virtual address dfffffc000000001
  KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
  batman_adv: bat0: Interface deactivated: brbh1337
  Mem abort info:
    ESR = 0x0000000096000004
    EC = 0x25: DABT (current EL), IL = 32 bits
    SET = 0, FnV = 0
    EA = 0, S1PTW = 0
    FSC = 0x04: level 0 translation fault
  Data abort info:
    ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
    CM = 0, WnR = 0, TnD = 0, TagAccess = 0
    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
  [dfffffc000000001] address between user and kernel address ranges
  Internal error: Oops: 0000000096000004 [#1] SMP
  CPU: 1 UID: 0 PID: 978 Comm: lbd Not tainted 6.13.0-g633f875b8f1e #114
  Hardware name: HW (DT)
  pstate: 10000005 (nzcV daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : ath12k_mac_op_tx+0x6cc/0x29b8 [ath12k]
  lr : ath12k_mac_op_tx+0x174/0x29b8 [ath12k]
  sp : ffffffc086ace450
  x29: ffffffc086ace450 x28: 0000000000000000 x27: 1ffffff810d59ca4
  x26: ffffff801d05f7c0 x25: 0000000000000000 x24: 000000004000001e
  x23: ffffff8009ce4926 x22: ffffff801f9c0800 x21: ffffff801d05f7f0
  x20: ffffff8034a19f40 x19: 0000000000000000 x18: ffffff801f9c0958
  x17: ffffff800bc0a504 x16: dfffffc000000000 x15: ffffffc086ace4f8
  x14: ffffff801d05f83c x13: 0000000000000000 x12: ffffffb003a0bf03
  x11: 0000000000000000 x10: ffffffb003a0bf02 x9 : ffffff8034a19f40
  x8 : ffffff801d05f818 x7 : 1ffffff0069433dc x6 : ffffff8034a19ee0
  x5 : ffffff801d05f7f0 x4 : 0000000000000000 x3 : 0000000000000001
  x2 : 0000000000000000 x1 : dfffffc000000000 x0 : 0000000000000008
  Call trace:
   ath12k_mac_op_tx+0x6cc/0x29b8 [ath12k] (P)
   ieee80211_handle_wake_tx_queue+0x16c/0x260
   ieee80211_queue_skb+0xeec/0x1d20
   ieee80211_tx+0x200/0x2c8
   ieee80211_xmit+0x22c/0x338
   __ieee80211_subif_start_xmit+0x7e8/0xc60
   ieee80211_subif_start_xmit+0xc4/0xee0
   __ieee80211_subif_start_xmit_8023.isra.0+0x854/0x17a0
   ieee80211_subif_start_xmit_8023+0x124/0x488
   dev_hard_start_xmit+0x160/0x5a8
   __dev_queue_xmit+0x6f8/0x3120
   br_dev_queue_push_xmit+0x120/0x4a8
   __br_forward+0xe4/0x2b0
   deliver_clone+0x5c/0xd0
   br_flood+0x398/0x580
   br_dev_xmit+0x454/0x9f8
   dev_hard_start_xmit+0x160/0x5a8
   __dev_queue_xmit+0x6f8/0x3120
   ip6_finish_output2+0xc28/0x1b60
   __ip6_finish_output+0x38c/0x638
   ip6_output+0x1b4/0x338
   ip6_local_out+0x7c/0xa8
   ip6_send_skb+0x7c/0x1b0
   ip6_push_pending_frames+0x94/0xd0
   rawv6_sendmsg+0x1a98/0x2898
   inet_sendmsg+0x94/0xe0
   __sys_sendto+0x1e4/0x308
   __arm64_sys_sendto+0xc4/0x140
   do_el0_svc+0x110/0x280
   el0_svc+0x20/0x60
   el0t_64_sync_handler+0x104/0x138
   el0t_64_sync+0x154/0x158

To avoid that, empty vif's txq at ieee80211_do_stop() so no packet could
be dequeued after ieee80211_do_stop() (new packets cannot be queued
because SDATA_STATE_RUNNING is cleared at this point).

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 net/mac80211/iface.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 738de269e13f..e60c1ffebaea 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -660,6 +660,9 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_do
 	if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
 		ieee80211_txq_remove_vlan(local, sdata);
 
+	if (sdata->vif.txq)
+		ieee80211_txq_purge(sdata->local, to_txq_info(sdata->vif.txq));
+
 	sdata->bss = NULL;
 
 	if (local->open_count == 0)
-- 
2.40.0

Re: [PATCH 2/2] wifi: mac80211: Purge vif txq in ieee80211_do_stop()

[Johannes Berg]

On Fri, 2025-03-14 at 12:04 +0100, Remi Pommarel wrote:
> After ieee80211_do_stop() SKB from vif's txq could still be processed.
> Indeed another concurrent vif schedule_and_wake_txq call could cause
> those packets to be dequeued (see ieee80211_handle_wake_tx_queue())
> without checking the sdata current state.
> 
> Because vif.drv_priv is now cleared in this function, this could lead to
> driver crash.
> 
> For example in ath12k, ahvif is store in vif.drv_priv. Thus if
> ath12k_mac_op_tx() is called after ieee80211_do_stop(), ahvif->ah can be
> NULL, leading the ath12k_warn(ahvif->ah,...) call in this function to
> trigger the NULL deref below.
> 
>   Unable to handle kernel paging request at virtual address dfffffc000000001
> 

Also here, can you find a Fixes: tag?

johannes

Re: [PATCH 0/2] Fixes packet processes after vif is stopped

[Remi Pommarel]

On Fri, Mar 14, 2025 at 12:04:23PM +0100, Remi Pommarel wrote:
> Those are a couple of fixes that prevent crashes due to processing
> packets (especially multicast ones) for TX after vif is stopped (either
> after a mesh interface left the group or interface is put down).
> 
> The first one ensure the key info passed to drivers through ieee80211
> skb control block is up to date, even after key removal.
> 
> The second one ensure no packets get processed after vif driver private
> data is cleared in ieee80211_do_stop().
> 
> As I tried to explain in second patch footnote, I can still see a
> theoretical reason that packets get queued after ieee80211_do_stop()
> call. But I was not able to reproduce it, so I may be missing a
> something here; making that more as an open question.

And I forgot to include the footnote in Patch 2/2. I was worried that
because the rcu_read_lock() in __ieee80211_subif_start_xmit() is taken
only after the sdata running state it could create a small window during
which a packet could still be enqueued passed the synchronize_rcu() of
ieee80211_do_stop(). But after digging a bit more, it seems that
all __ieee80211_subif_start_xmit() callers (e.g. __dev_queue_xmit())
take the rcu_read_lock() already. So please ignore this last remark.

Regards,

-- 
Remi