| Message ID | 95269f93724a94ee0b22f8107fe5b5e8f2fbea76.1741950009.git.repk@triplefau.lt |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes packet processes after vif is stopped | expand |
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
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 ?
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.
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
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
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;
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(+)