| Message ID | 20220928193057.16132-1-greearb@candelatech.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: iwlwifi: fix double free on tx path. | expand |
On Wed, 2022-09-28 at 12:30 -0700, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > We see kernel crashes and lockups and KASAN errors related to ax210 > firmware crashes. One of the KASAN dumps pointed at the tx path, > and it appears there is indeed a way to double-free an skb. > > If iwl_mvm_tx_skb_sta returns non-zero, then the 'skb' sent into the > method will be freed. But, in case where we build TSO skb buffer, > the skb may also be freed in error case. So, return 0 in that particular > error case and do cleanup manually. > > BUG: KASAN: use-after-free in __list_del_entry_valid+0x12/0x90 > iwlwifi 0000:06:00.0: 0x00000000 | tsf hi > Read of size 8 at addr ffff88813cfa4ba0 by task btserver/9650 > > CPU: 4 PID: 9650 Comm: btserver Tainted: G W 5.19.8+ #5 > iwlwifi 0000:06:00.0: 0x00000000 | time gp1 > Hardware name: Default string Default string/SKYBAY, BIOS 5.12 02/19/2019 > Call Trace: > <TASK> > dump_stack_lvl+0x55/0x6d > print_report.cold.12+0xf2/0x684 > iwlwifi 0000:06:00.0: 0x1D0915A8 | time gp2 > ? __list_del_entry_valid+0x12/0x90 > kasan_report+0x8b/0x180 > iwlwifi 0000:06:00.0: 0x00000001 | uCode revision type > ? __list_del_entry_valid+0x12/0x90 > __list_del_entry_valid+0x12/0x90 > iwlwifi 0000:06:00.0: 0x00000048 | uCode version major > tcp_update_skb_after_send+0x5d/0x170 > __tcp_transmit_skb+0xb61/0x15c0 > iwlwifi 0000:06:00.0: 0xDAA05125 | uCode version minor > ? __tcp_select_window+0x490/0x490 > iwlwifi 0000:06:00.0: 0x00000420 | hw version > ? trace_kmalloc_node+0x29/0xd0 > ? __kmalloc_node_track_caller+0x12a/0x260 > ? memset+0x1f/0x40 > ? __build_skb_around+0x125/0x150 > ? __alloc_skb+0x1d4/0x220 > ? skb_zerocopy_clone+0x55/0x230 > iwlwifi 0000:06:00.0: 0x00489002 | board version > ? kmalloc_reserve+0x80/0x80 > ? rcu_read_lock_bh_held+0x60/0xb0 > tcp_write_xmit+0x3f1/0x24d0 > iwlwifi 0000:06:00.0: 0x034E001C | hcmd > ? __check_object_size+0x180/0x350 > iwlwifi 0000:06:00.0: 0x24020000 | isr0 > tcp_sendmsg_locked+0x8a9/0x1520 > iwlwifi 0000:06:00.0: 0x01400000 | isr1 > ? tcp_sendpage+0x50/0x50 > iwlwifi 0000:06:00.0: 0x48F0000A | isr2 > ? lock_release+0xb9/0x400 > ? tcp_sendmsg+0x14/0x40 > iwlwifi 0000:06:00.0: 0x00C3080C | isr3 > ? lock_downgrade+0x390/0x390 > ? do_raw_spin_lock+0x114/0x1d0 > iwlwifi 0000:06:00.0: 0x00200000 | isr4 > ? rwlock_bug.part.2+0x50/0x50 > iwlwifi 0000:06:00.0: 0x034A001C | last cmd Id > ? rwlock_bug.part.2+0x50/0x50 > ? lockdep_hardirqs_on_prepare+0xe/0x200 > iwlwifi 0000:06:00.0: 0x0000C2F0 | wait_event > ? __local_bh_enable_ip+0x87/0xe0 > ? inet_send_prepare+0x220/0x220 > iwlwifi 0000:06:00.0: 0x000000C4 | l2p_control > tcp_sendmsg+0x22/0x40 > sock_sendmsg+0x5f/0x70 > iwlwifi 0000:06:00.0: 0x00010034 | l2p_duration > __sys_sendto+0x19d/0x250 > iwlwifi 0000:06:00.0: 0x00000007 | l2p_mhvalid > ? __ia32_sys_getpeername+0x40/0x40 > iwlwifi 0000:06:00.0: 0x00000000 | l2p_addr_match > ? rcu_read_lock_held_common+0x12/0x50 > ? rcu_read_lock_sched_held+0x5a/0xd0 > ? rcu_read_lock_bh_held+0xb0/0xb0 > ? rcu_read_lock_sched_held+0x5a/0xd0 > ? rcu_read_lock_sched_held+0x5a/0xd0 > ? lock_release+0xb9/0x400 > ? lock_downgrade+0x390/0x390 > ? ktime_get+0x64/0x130 > ? ktime_get+0x8d/0x130 > ? rcu_read_lock_held_common+0x12/0x50 > ? rcu_read_lock_sched_held+0x5a/0xd0 > ? rcu_read_lock_held_common+0x12/0x50 > ? rcu_read_lock_sched_held+0x5a/0xd0 > ? rcu_read_lock_bh_held+0xb0/0xb0 > ? rcu_read_lock_bh_held+0xb0/0xb0 > __x64_sys_sendto+0x6f/0x80 > do_syscall_64+0x34/0xb0 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > RIP: 0033:0x7f1d126e4531 > Code: 00 00 00 00 0f 1f 44 00 00 f3 0f 1e fa 48 8d 05 35 80 0c 00 41 89 ca 8b 00 85 c0 75 1c 45 31 c9 45 31 c0 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 67 c3 66 0f 1f 44 00 00 55 48 83 ec 20 48 > 89 > RSP: 002b:00007ffe21a679d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002c > RAX: ffffffffffffffda RBX: 000000000000ffdc RCX: 00007f1d126e4531 > RDX: 0000000000010000 RSI: 000000000374acf0 RDI: 0000000000000014 > RBP: 00007ffe21a67ac0 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000010 > R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000000 > </TASK> > > Allocated by task 9650: > kasan_save_stack+0x1c/0x40 > __kasan_slab_alloc+0x6d/0x90 > kmem_cache_alloc_node+0xf3/0x2b0 > __alloc_skb+0x191/0x220 > tcp_stream_alloc_skb+0x3f/0x330 > tcp_sendmsg_locked+0x67c/0x1520 > tcp_sendmsg+0x22/0x40 > sock_sendmsg+0x5f/0x70 > __sys_sendto+0x19d/0x250 > __x64_sys_sendto+0x6f/0x80 > do_syscall_64+0x34/0xb0 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Freed by task 9650: > kasan_save_stack+0x1c/0x40 > kasan_set_track+0x21/0x30 > kasan_set_free_info+0x20/0x30 > __kasan_slab_free+0x102/0x170 > kmem_cache_free+0xc8/0x3e0 > iwl_mvm_mac_itxq_xmit+0x124/0x270 [iwlmvm] > ieee80211_queue_skb+0x874/0xd10 [mac80211] > ieee80211_xmit_fast+0xf80/0x1180 [mac80211] > __ieee80211_subif_start_xmit+0x287/0x680 [mac80211] > ieee80211_subif_start_xmit+0xcd/0x730 [mac80211] > dev_hard_start_xmit+0xf6/0x420 > __dev_queue_xmit+0x165b/0x1b50 > ip_finish_output2+0x66e/0xfb0 > __ip_finish_output+0x487/0x6d0 > ip_output+0x11c/0x350 > __ip_queue_xmit+0x36b/0x9d0 > __tcp_transmit_skb+0xb35/0x15c0 > tcp_write_xmit+0x3f1/0x24d0 > tcp_sendmsg_locked+0x8a9/0x1520 > tcp_sendmsg+0x22/0x40 > sock_sendmsg+0x5f/0x70 > __sys_sendto+0x19d/0x250 > __x64_sys_sendto+0x6f/0x80 > do_syscall_64+0x34/0xb0 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > The buggy address belongs to the object at ffff88813cfa4b40 > which belongs to the cache skbuff_fclone_cache of size 472 > The buggy address is located 96 bytes inside of > 472-byte region [ffff88813cfa4b40, ffff88813cfa4d18) > > The buggy address belongs to the physical page: > page:ffffea0004f3e900 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88813cfa6c40 pfn:0x13cfa4 > head:ffffea0004f3e900 order:2 compound_mapcount:0 compound_pincount:0 > flags: 0x5fff8000010200(slab|head|node=0|zone=2|lastcpupid=0x3fff) > raw: 005fff8000010200 ffffea0004656b08 ffffea0008e8cf08 ffff8881081a5240 > raw: ffff88813cfa6c40 0000000000170015 00000001ffffffff 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff88813cfa4a80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > ffff88813cfa4b00: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb > > ffff88813cfa4b80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff88813cfa4c00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88813cfa4c80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== > > Tested-by: Amol Jawale <amol.jawale@candelatech.com> > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > index f9e08b339e0c..72bba83b4603 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c > @@ -1206,6 +1206,7 @@ int iwl_mvm_tx_skb_sta(struct iwl_mvm *mvm, struct sk_buff *skb, > struct sk_buff_head mpdus_skbs; > unsigned int payload_len; > int ret; > + struct sk_buff *orig_skb = skb; > > if (WARN_ON_ONCE(!mvmsta)) > return -1; > @@ -1238,8 +1239,17 @@ int iwl_mvm_tx_skb_sta(struct iwl_mvm *mvm, struct sk_buff *skb, > > ret = iwl_mvm_tx_mpdu(mvm, skb, &info, sta); > if (ret) { Maybe while on it, add here "if (unlikely(ret)) {"? > + /* Free skbs created as part of TSO logic that have not yet been dequeued */ > __skb_queue_purge(&mpdus_skbs); > - return ret; > + /* skb here is not necessarily same as skb that entered this method, > + * so free it explicitly. > + */ > + if (skb == orig_skb) > + ieee80211_free_txskb(mvm->hw, skb); > + else > + kfree_skb(skb); > + /* there was error, but we consumed skb one way or another, so return 0 */ > + return 0; > } > } > Thanks for the fix! Gregory
On 10/12/22 7:17 AM, Greenman, Gregory wrote: >> if (WARN_ON_ONCE(!mvmsta)) >> return -1; >> @@ -1238,8 +1239,17 @@ int iwl_mvm_tx_skb_sta(struct iwl_mvm *mvm, struct sk_buff *skb, >> >> ret = iwl_mvm_tx_mpdu(mvm, skb, &info, sta); >> if (ret) { > > Maybe while on it, add here "if (unlikely(ret)) {"? I don't think it is worth respinning the patch for that, but could add a follow-on patch. Thanks, Ben > >> + /* Free skbs created as part of TSO logic that have not yet been dequeued */ >> __skb_queue_purge(&mpdus_skbs); >> - return ret; >> + /* skb here is not necessarily same as skb that entered this method, >> + * so free it explicitly. >> + */ >> + if (skb == orig_skb) >> + ieee80211_free_txskb(mvm->hw, skb); >> + else >> + kfree_skb(skb); >> + /* there was error, but we consumed skb one way or another, so return 0 */ >> + return 0; >> } >> } >> > > Thanks for the fix! > Gregory >
On 10/12/22 7:17 AM, Greenman, Gregory wrote: > On Wed, 2022-09-28 at 12:30 -0700, greearb@candelatech.com wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> We see kernel crashes and lockups and KASAN errors related to ax210 >> firmware crashes. One of the KASAN dumps pointed at the tx path, >> and it appears there is indeed a way to double-free an skb. While rebasing on top of 6.1-rc4, I notice this patch is not in the tree yet. I think it is worth adding it to 6.1 since it is a pretty nasty kernel crash... Thanks, Ben
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c index f9e08b339e0c..72bba83b4603 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c @@ -1206,6 +1206,7 @@ int iwl_mvm_tx_skb_sta(struct iwl_mvm *mvm, struct sk_buff *skb, struct sk_buff_head mpdus_skbs; unsigned int payload_len; int ret; + struct sk_buff *orig_skb = skb; if (WARN_ON_ONCE(!mvmsta)) return -1; @@ -1238,8 +1239,17 @@ int iwl_mvm_tx_skb_sta(struct iwl_mvm *mvm, struct sk_buff *skb, ret = iwl_mvm_tx_mpdu(mvm, skb, &info, sta); if (ret) { + /* Free skbs created as part of TSO logic that have not yet been dequeued */ __skb_queue_purge(&mpdus_skbs); - return ret; + /* skb here is not necessarily same as skb that entered this method, + * so free it explicitly. + */ + if (skb == orig_skb) + ieee80211_free_txskb(mvm->hw, skb); + else + kfree_skb(skb); + /* there was error, but we consumed skb one way or another, so return 0 */ + return 0; } }