Fixes packet processes after vif is stopped

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

Posted by Remi Pommarel 9 months, 1 week ago

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()

Posted by Johannes Berg 8 months, 4 weeks ago

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()

Posted by Remi Pommarel 8 months, 4 weeks ago

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()

Posted by Johannes Berg 8 months, 4 weeks ago

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()

Posted by Toke Høiland-Jørgensen 8 months, 3 weeks ago

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

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

Posted by Remi Pommarel 8 months, 4 weeks ago

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