* KMSAN: uninit-value in ath9k_htc_rx_msg @ 2020-08-09 16:27 syzbot 2020-08-13 3:32 ` syzbot 2022-07-30 12:13 ` [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() Tetsuo Handa 0 siblings, 2 replies; 17+ messages in thread From: syzbot @ 2020-08-09 16:27 UTC (permalink / raw) To: ath9k-devel, davem, glider, kuba, kvalo, linux-kernel, linux-wireless, netdev, syzkaller-bugs Hello, syzbot found the following issue on: HEAD commit: ce8056d1 wip: changed copy_from_user where instrumented git tree: https://github.com/google/kmsan.git master console output: https://syzkaller.appspot.com/x/log.txt?x=130c562c900000 kernel config: https://syzkaller.appspot.com/x/.config?x=3afe005fb99591f dashboard link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81) userspace arch: i386 Unfortunately, I don't have any reproducer for this issue yet. IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com ===================================================== BUG: KMSAN: uninit-value in ath9k_htc_rx_msg+0x28f/0x1f50 drivers/net/wireless/ath/ath9k/htc_hst.c:410 CPU: 0 PID: 4867 Comm: systemd-udevd Not tainted 5.8.0-rc5-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: <IRQ> __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x21c/0x280 lib/dump_stack.c:118 kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:121 __msan_warning+0x58/0xa0 mm/kmsan/kmsan_instr.c:215 ath9k_htc_rx_msg+0x28f/0x1f50 drivers/net/wireless/ath/ath9k/htc_hst.c:410 ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:638 [inline] ath9k_hif_usb_rx_cb+0x1841/0x1d10 drivers/net/wireless/ath/ath9k/hif_usb.c:671 __usb_hcd_giveback_urb+0x687/0x870 drivers/usb/core/hcd.c:1650 usb_hcd_giveback_urb+0x1cb/0x730 drivers/usb/core/hcd.c:1716 dummy_timer+0xd98/0x71c0 drivers/usb/gadget/udc/dummy_hcd.c:1967 call_timer_fn+0x226/0x550 kernel/time/timer.c:1404 expire_timers+0x4fc/0x780 kernel/time/timer.c:1449 __run_timers+0xaf4/0xd30 kernel/time/timer.c:1773 run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786 __do_softirq+0x2ea/0x7f5 kernel/softirq.c:293 asm_call_on_stack+0xf/0x20 arch/x86/entry/entry_64.S:711 </IRQ> __run_on_irqstack arch/x86/include/asm/irq_stack.h:23 [inline] run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:50 [inline] do_softirq_own_stack+0x7c/0xa0 arch/x86/kernel/irq_64.c:77 invoke_softirq kernel/softirq.c:390 [inline] __irq_exit_rcu+0x226/0x270 kernel/softirq.c:420 irq_exit_rcu+0xe/0x10 kernel/softirq.c:432 sysvec_apic_timer_interrupt+0x107/0x130 arch/x86/kernel/apic/apic.c:1091 asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:593 RIP: 0010:tomoyo_path_matches_pattern+0xba/0x4d0 security/tomoyo/util.c:920 Code: 48 89 45 a0 8b 02 89 45 d0 4d 85 f6 4c 8b ad 70 ff ff ff 0f 85 c6 01 00 00 4c 89 7d b8 41 0f b7 44 24 0c 48 89 45 80 48 89 df <e8> 71 fd ea fc 49 8d 5c 24 0f 0f b7 00 48 89 45 88 8b 02 89 45 a8 RSP: 0018:ffff888111c0f858 EFLAGS: 00000246 RAX: 0000000000000015 RBX: ffff888111d41fe4 RCX: 0000000111941fd8 RDX: ffff888111941fd8 RSI: 0000000000000440 RDI: ffff888111d41fe4 RBP: ffff888111c0f8e8 R08: ffffea000000000f R09: ffff88812fffa000 R10: 0000000000000003 R11: ffff88811a320000 R12: ffff888111d41fd8 R13: ffff88811a3209d8 R14: 0000000000000000 R15: ffff888111c0fab0 tomoyo_compare_name_union security/tomoyo/file.c:87 [inline] tomoyo_check_path_acl+0x272/0x360 security/tomoyo/file.c:260 tomoyo_check_acl+0x239/0x5a0 security/tomoyo/domain.c:175 tomoyo_path_permission security/tomoyo/file.c:586 [inline] tomoyo_path_perm+0x83f/0xc60 security/tomoyo/file.c:838 tomoyo_inode_getattr+0x54/0x60 security/tomoyo/tomoyo.c:123 security_inode_getattr+0x144/0x280 security/security.c:1278 vfs_getattr fs/stat.c:121 [inline] vfs_statx+0x34c/0x940 fs/stat.c:206 vfs_lstat include/linux/fs.h:3302 [inline] __do_sys_newlstat fs/stat.c:374 [inline] __se_sys_newlstat+0xce/0x920 fs/stat.c:368 __x64_sys_newlstat+0x3e/0x60 fs/stat.c:368 do_syscall_64+0xad/0x160 arch/x86/entry/common.c:386 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f72dd774335 Code: Bad RIP value. RSP: 002b:00007ffeb28263b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000006 RAX: ffffffffffffffda RBX: 000055a2c06613f0 RCX: 00007f72dd774335 RDX: 00007ffeb28263f0 RSI: 00007ffeb28263f0 RDI: 000055a2c06603f0 RBP: 00007ffeb28264b0 R08: 00007f72dda33178 R09: 0000000000001010 R10: 0000000000000020 R11: 0000000000000246 R12: 000055a2c06603f0 R13: 000055a2c0660410 R14: 000055a2c064ce5b R15: 000055a2c064ce60 Uninit was created at: kmsan_save_stack_with_flags+0x3c/0x90 mm/kmsan/kmsan.c:144 kmsan_internal_alloc_meta_for_pages mm/kmsan/kmsan_shadow.c:269 [inline] kmsan_alloc_page+0xc5/0x1a0 mm/kmsan/kmsan_shadow.c:293 __alloc_pages_nodemask+0xdf0/0x1030 mm/page_alloc.c:4889 __alloc_pages include/linux/gfp.h:509 [inline] __alloc_pages_node include/linux/gfp.h:522 [inline] alloc_pages_node include/linux/gfp.h:536 [inline] __page_frag_cache_refill mm/page_alloc.c:4964 [inline] page_frag_alloc+0x35b/0x880 mm/page_alloc.c:4994 __netdev_alloc_skb+0x2a8/0xc90 net/core/skbuff.c:451 __dev_alloc_skb include/linux/skbuff.h:2813 [inline] ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:620 [inline] ath9k_hif_usb_rx_cb+0xe5a/0x1d10 drivers/net/wireless/ath/ath9k/hif_usb.c:671 __usb_hcd_giveback_urb+0x687/0x870 drivers/usb/core/hcd.c:1650 usb_hcd_giveback_urb+0x1cb/0x730 drivers/usb/core/hcd.c:1716 dummy_timer+0xd98/0x71c0 drivers/usb/gadget/udc/dummy_hcd.c:1967 call_timer_fn+0x226/0x550 kernel/time/timer.c:1404 expire_timers+0x4fc/0x780 kernel/time/timer.c:1449 __run_timers+0xaf4/0xd30 kernel/time/timer.c:1773 run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786 __do_softirq+0x2ea/0x7f5 kernel/softirq.c:293 ===================================================== --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2020-08-09 16:27 KMSAN: uninit-value in ath9k_htc_rx_msg syzbot @ 2020-08-13 3:32 ` syzbot [not found] ` <a142d63c-7810-40ff-9c24-7160c63bafebn@googlegroups.com> 2022-07-30 12:13 ` [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() Tetsuo Handa 1 sibling, 1 reply; 17+ messages in thread From: syzbot @ 2020-08-13 3:32 UTC (permalink / raw) To: ath9k-devel, davem, glider, kuba, kvalo, linux-kernel, linux-wireless, netdev, syzkaller-bugs syzbot has found a reproducer for the following issue on: HEAD commit: ce8056d1 wip: changed copy_from_user where instrumented git tree: https://github.com/google/kmsan.git master console output: https://syzkaller.appspot.com/x/log.txt?x=12985a16900000 kernel config: https://syzkaller.appspot.com/x/.config?x=3afe005fb99591f dashboard link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1468efe2900000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10bb9fba900000 IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com ===================================================== BUG: KMSAN: uninit-value in ath9k_htc_rx_msg+0x28f/0x1f50 drivers/net/wireless/ath/ath9k/htc_hst.c:410 CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.8.0-rc5-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: <IRQ> __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x21c/0x280 lib/dump_stack.c:118 kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:121 __msan_warning+0x58/0xa0 mm/kmsan/kmsan_instr.c:215 ath9k_htc_rx_msg+0x28f/0x1f50 drivers/net/wireless/ath/ath9k/htc_hst.c:410 ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:638 [inline] ath9k_hif_usb_rx_cb+0x1841/0x1d10 drivers/net/wireless/ath/ath9k/hif_usb.c:671 __usb_hcd_giveback_urb+0x687/0x870 drivers/usb/core/hcd.c:1650 usb_hcd_giveback_urb+0x1cb/0x730 drivers/usb/core/hcd.c:1716 dummy_timer+0xd98/0x71c0 drivers/usb/gadget/udc/dummy_hcd.c:1967 call_timer_fn+0x226/0x550 kernel/time/timer.c:1404 expire_timers+0x4fc/0x780 kernel/time/timer.c:1449 __run_timers+0xaf4/0xd30 kernel/time/timer.c:1773 run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786 __do_softirq+0x2ea/0x7f5 kernel/softirq.c:293 asm_call_on_stack+0xf/0x20 arch/x86/entry/entry_64.S:711 </IRQ> __run_on_irqstack arch/x86/include/asm/irq_stack.h:23 [inline] run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:50 [inline] do_softirq_own_stack+0x7c/0xa0 arch/x86/kernel/irq_64.c:77 invoke_softirq kernel/softirq.c:390 [inline] __irq_exit_rcu+0x226/0x270 kernel/softirq.c:420 irq_exit_rcu+0xe/0x10 kernel/softirq.c:432 sysvec_apic_timer_interrupt+0x107/0x130 arch/x86/kernel/apic/apic.c:1091 asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:593 RIP: 0010:native_irq_disable arch/x86/include/asm/irqflags.h:49 [inline] RIP: 0010:arch_local_irq_disable arch/x86/include/asm/irqflags.h:89 [inline] RIP: 0010:acpi_safe_halt drivers/acpi/processor_idle.c:112 [inline] RIP: 0010:acpi_idle_do_entry drivers/acpi/processor_idle.c:525 [inline] RIP: 0010:acpi_idle_enter+0x817/0xeb0 drivers/acpi/processor_idle.c:651 Code: 85 db 74 0a f7 d3 44 21 fb 48 85 db 74 32 4d 85 ff 75 3a 48 8b 5d a0 e9 0c 00 00 00 e8 12 b2 78 fb 0f 00 2d 25 15 1c 0b fb f4 <fa> eb 5a 84 c0 8b 7d 90 0f 45 7d 94 e8 d8 9a f4 fb e9 74 fc ff ff RSP: 0018:ffff88812df93bc8 EFLAGS: 00000246 RAX: 0000000000000000 RBX: ffff8881dfefce70 RCX: 000000012db88000 RDX: ffff88812df88000 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffff88812df93ca0 R08: ffffffff86420acc R09: ffff88812fffa000 R10: 0000000000000002 R11: ffff88812df88000 R12: ffff88812df889d8 R13: ffff8881dfefcc64 R14: 0000000000000000 R15: 0000000000000000 cpuidle_enter_state+0x860/0x12b0 drivers/cpuidle/cpuidle.c:235 cpuidle_enter+0xe3/0x170 drivers/cpuidle/cpuidle.c:346 call_cpuidle kernel/sched/idle.c:126 [inline] cpuidle_idle_call kernel/sched/idle.c:214 [inline] do_idle+0x668/0x810 kernel/sched/idle.c:276 cpu_startup_entry+0x45/0x50 kernel/sched/idle.c:372 start_secondary+0x1bf/0x240 arch/x86/kernel/smpboot.c:268 secondary_startup_64+0xa4/0xb0 arch/x86/kernel/head_64.S:243 Uninit was created at: kmsan_save_stack_with_flags+0x3c/0x90 mm/kmsan/kmsan.c:144 kmsan_internal_alloc_meta_for_pages mm/kmsan/kmsan_shadow.c:269 [inline] kmsan_alloc_page+0xc5/0x1a0 mm/kmsan/kmsan_shadow.c:293 __alloc_pages_nodemask+0xdf0/0x1030 mm/page_alloc.c:4889 __alloc_pages include/linux/gfp.h:509 [inline] __alloc_pages_node include/linux/gfp.h:522 [inline] alloc_pages_node include/linux/gfp.h:536 [inline] __page_frag_cache_refill mm/page_alloc.c:4964 [inline] page_frag_alloc+0x35b/0x880 mm/page_alloc.c:4994 __netdev_alloc_skb+0x2a8/0xc90 net/core/skbuff.c:451 __dev_alloc_skb include/linux/skbuff.h:2813 [inline] ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:620 [inline] ath9k_hif_usb_rx_cb+0xe5a/0x1d10 drivers/net/wireless/ath/ath9k/hif_usb.c:671 __usb_hcd_giveback_urb+0x687/0x870 drivers/usb/core/hcd.c:1650 usb_hcd_giveback_urb+0x1cb/0x730 drivers/usb/core/hcd.c:1716 dummy_timer+0xd98/0x71c0 drivers/usb/gadget/udc/dummy_hcd.c:1967 call_timer_fn+0x226/0x550 kernel/time/timer.c:1404 expire_timers+0x4fc/0x780 kernel/time/timer.c:1449 __run_timers+0xaf4/0xd30 kernel/time/timer.c:1773 run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786 __do_softirq+0x2ea/0x7f5 kernel/softirq.c:293 ===================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <a142d63c-7810-40ff-9c24-7160c63bafebn@googlegroups.com>]
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg [not found] ` <a142d63c-7810-40ff-9c24-7160c63bafebn@googlegroups.com> @ 2022-08-24 13:30 ` Alexander Potapenko 2022-08-25 14:34 ` Tetsuo Handa 0 siblings, 1 reply; 17+ messages in thread From: Alexander Potapenko @ 2022-08-24 13:30 UTC (permalink / raw) To: syzkaller-bugs Cc: ath9k-devel, David Miller, Jakub Kicinski, kvalo, LKML, linux-wireless, Networking, phil (adding back people originally CCed on the syzkaller bug. Unfortunately it isn't possible to reply to all in Google Groups) On Wed, Aug 24, 2022 at 3:26 PM Alexander Potapenko <glider@google.com> wrote: > > > > On Thursday, August 13, 2020 at 5:32:17 AM UTC+2 syzbot wrote: >> >> syzbot has found a reproducer for the following issue on: >> >> HEAD commit: ce8056d1 wip: changed copy_from_user where instrumented >> git tree: https://github.com/google/kmsan.git master >> console output: https://syzkaller.appspot.com/x/log.txt?x=12985a16900000 >> kernel config: https://syzkaller.appspot.com/x/.config?x=3afe005fb99591f >> dashboard link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f >> compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81) >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1468efe2900000 >> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10bb9fba900000 >> >> IMPORTANT: if you fix the issue, please add the following tag to the commit: >> Reported-by: syzbot+2ca247...@syzkaller.appspotmail.com >> >> ===================================================== >> BUG: KMSAN: uninit-value in ath9k_htc_rx_msg+0x28f/0x1f50 drivers/net/wireless/ath/ath9k/htc_hst.c:410 >> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.8.0-rc5-syzkaller #0 >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 >> Call Trace: >> <IRQ> >> __dump_stack lib/dump_stack.c:77 [inline] >> dump_stack+0x21c/0x280 lib/dump_stack.c:118 >> kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:121 >> __msan_warning+0x58/0xa0 mm/kmsan/kmsan_instr.c:215 >> ath9k_htc_rx_msg+0x28f/0x1f50 drivers/net/wireless/ath/ath9k/htc_hst.c:410 >> ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:638 [inline] >> ath9k_hif_usb_rx_cb+0x1841/0x1d10 drivers/net/wireless/ath/ath9k/hif_usb.c:671 >> __usb_hcd_giveback_urb+0x687/0x870 drivers/usb/core/hcd.c:1650 >> usb_hcd_giveback_urb+0x1cb/0x730 drivers/usb/core/hcd.c:1716 >> dummy_timer+0xd98/0x71c0 drivers/usb/gadget/udc/dummy_hcd.c:1967 >> call_timer_fn+0x226/0x550 kernel/time/timer.c:1404 >> expire_timers+0x4fc/0x780 kernel/time/timer.c:1449 >> __run_timers+0xaf4/0xd30 kernel/time/timer.c:1773 >> run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786 >> __do_softirq+0x2ea/0x7f5 kernel/softirq.c:293 >> asm_call_on_stack+0xf/0x20 arch/x86/entry/entry_64.S:711 >> </IRQ> >> __run_on_irqstack arch/x86/include/asm/irq_stack.h:23 [inline] >> run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:50 [inline] >> do_softirq_own_stack+0x7c/0xa0 arch/x86/kernel/irq_64.c:77 >> invoke_softirq kernel/softirq.c:390 [inline] >> __irq_exit_rcu+0x226/0x270 kernel/softirq.c:420 >> irq_exit_rcu+0xe/0x10 kernel/softirq.c:432 >> sysvec_apic_timer_interrupt+0x107/0x130 arch/x86/kernel/apic/apic.c:1091 >> asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:593 >> RIP: 0010:native_irq_disable arch/x86/include/asm/irqflags.h:49 [inline] >> RIP: 0010:arch_local_irq_disable arch/x86/include/asm/irqflags.h:89 [inline] >> RIP: 0010:acpi_safe_halt drivers/acpi/processor_idle.c:112 [inline] >> RIP: 0010:acpi_idle_do_entry drivers/acpi/processor_idle.c:525 [inline] >> RIP: 0010:acpi_idle_enter+0x817/0xeb0 drivers/acpi/processor_idle.c:651 >> Code: 85 db 74 0a f7 d3 44 21 fb 48 85 db 74 32 4d 85 ff 75 3a 48 8b 5d a0 e9 0c 00 00 00 e8 12 b2 78 fb 0f 00 2d 25 15 1c 0b fb f4 <fa> eb 5a 84 c0 8b 7d 90 0f 45 7d 94 e8 d8 9a f4 fb e9 74 fc ff ff >> RSP: 0018:ffff88812df93bc8 EFLAGS: 00000246 >> RAX: 0000000000000000 RBX: ffff8881dfefce70 RCX: 000000012db88000 >> RDX: ffff88812df88000 RSI: 0000000000000000 RDI: 0000000000000000 >> RBP: ffff88812df93ca0 R08: ffffffff86420acc R09: ffff88812fffa000 >> R10: 0000000000000002 R11: ffff88812df88000 R12: ffff88812df889d8 >> R13: ffff8881dfefcc64 R14: 0000000000000000 R15: 0000000000000000 >> cpuidle_enter_state+0x860/0x12b0 drivers/cpuidle/cpuidle.c:235 >> cpuidle_enter+0xe3/0x170 drivers/cpuidle/cpuidle.c:346 >> call_cpuidle kernel/sched/idle.c:126 [inline] >> cpuidle_idle_call kernel/sched/idle.c:214 [inline] >> do_idle+0x668/0x810 kernel/sched/idle.c:276 >> cpu_startup_entry+0x45/0x50 kernel/sched/idle.c:372 >> start_secondary+0x1bf/0x240 arch/x86/kernel/smpboot.c:268 >> secondary_startup_64+0xa4/0xb0 arch/x86/kernel/head_64.S:243 >> >> Uninit was created at: >> kmsan_save_stack_with_flags+0x3c/0x90 mm/kmsan/kmsan.c:144 >> kmsan_internal_alloc_meta_for_pages mm/kmsan/kmsan_shadow.c:269 [inline] >> kmsan_alloc_page+0xc5/0x1a0 mm/kmsan/kmsan_shadow.c:293 >> __alloc_pages_nodemask+0xdf0/0x1030 mm/page_alloc.c:4889 >> __alloc_pages include/linux/gfp.h:509 [inline] >> __alloc_pages_node include/linux/gfp.h:522 [inline] >> alloc_pages_node include/linux/gfp.h:536 [inline] >> __page_frag_cache_refill mm/page_alloc.c:4964 [inline] >> page_frag_alloc+0x35b/0x880 mm/page_alloc.c:4994 >> __netdev_alloc_skb+0x2a8/0xc90 net/core/skbuff.c:451 >> __dev_alloc_skb include/linux/skbuff.h:2813 [inline] >> ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:620 [inline] >> ath9k_hif_usb_rx_cb+0xe5a/0x1d10 drivers/net/wireless/ath/ath9k/hif_usb.c:671 >> __usb_hcd_giveback_urb+0x687/0x870 drivers/usb/core/hcd.c:1650 >> usb_hcd_giveback_urb+0x1cb/0x730 drivers/usb/core/hcd.c:1716 >> dummy_timer+0xd98/0x71c0 drivers/usb/gadget/udc/dummy_hcd.c:1967 >> call_timer_fn+0x226/0x550 kernel/time/timer.c:1404 >> expire_timers+0x4fc/0x780 kernel/time/timer.c:1449 >> __run_timers+0xaf4/0xd30 kernel/time/timer.c:1773 >> run_timer_softirq+0x2d/0x50 kernel/time/timer.c:1786 >> __do_softirq+0x2ea/0x7f5 kernel/softirq.c:293 >> ===================================================== >> > > This bug bites us quite often on syzbot: https://syzkaller.appspot.com/bug?id=659ddf411502a2fe220c8f9be696d5a8d8db726e (17k crashes) > The patch below by phil@philpotter.co.uk (https://syzkaller.appspot.com/text?tag=Patch&x=173dcb51d00000) seems to fix the problem, but I have no idea what's going on there. > > ============================================================== > diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c > index 510e61e97dbc..9dbfff7a388e 100644 > --- a/drivers/net/wireless/ath/ath9k/htc_hst.c > +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c > @@ -403,7 +403,7 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, > struct htc_endpoint *endpoint; > __be16 *msg_id; > > - if (!htc_handle || !skb) > + if (!htc_handle || !skb || !pskb_may_pull(skb, sizeof(struct htc_frame_hdr))) > return; > > htc_hdr = (struct htc_frame_hdr *) skb->data; > ============================================================== -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Liana Sebastian Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-24 13:30 ` Alexander Potapenko @ 2022-08-25 14:34 ` Tetsuo Handa 2022-08-25 15:09 ` Alexander Potapenko 0 siblings, 1 reply; 17+ messages in thread From: Tetsuo Handa @ 2022-08-25 14:34 UTC (permalink / raw) To: Alexander Potapenko, phil Cc: ath9k-devel, David Miller, Jakub Kicinski, Kalle Valo, LKML, linux-wireless, Networking, syzkaller-bugs Hello. I found that your patch was applied. But since the reproducer tested only 0 byte case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes is not sufficient. More complete patch with Ack from Toke is waiting at https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . Please consider overriding with my version. On 2022/08/24 22:30, Alexander Potapenko wrote: > (adding back people originally CCed on the syzkaller bug. > Unfortunately it isn't possible to reply to all in Google Groups) > > On Wed, Aug 24, 2022 at 3:26 PM Alexander Potapenko wrote: >> This bug bites us quite often on syzbot: https://syzkaller.appspot.com/bug?id=659ddf411502a2fe220c8f9be696d5a8d8db726e (17k crashes) >> The patch below by phil@philpotter.co.uk (https://syzkaller.appspot.com/text?tag=Patch&x=173dcb51d00000) seems to fix the problem, but I have no idea what's going on there. >> >> ============================================================== >> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c >> index 510e61e97dbc..9dbfff7a388e 100644 >> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c >> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c >> @@ -403,7 +403,7 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, >> struct htc_endpoint *endpoint; >> __be16 *msg_id; >> >> - if (!htc_handle || !skb) >> + if (!htc_handle || !skb || !pskb_may_pull(skb, sizeof(struct htc_frame_hdr))) >> return; >> >> htc_hdr = (struct htc_frame_hdr *) skb->data; >> ============================================================== > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-25 14:34 ` Tetsuo Handa @ 2022-08-25 15:09 ` Alexander Potapenko 2022-08-25 15:55 ` Toke Høiland-Jørgensen 2022-08-26 1:35 ` Tetsuo Handa 0 siblings, 2 replies; 17+ messages in thread From: Alexander Potapenko @ 2022-08-25 15:09 UTC (permalink / raw) To: Tetsuo Handa Cc: phil, ath9k-devel, David Miller, Jakub Kicinski, Kalle Valo, LKML, linux-wireless, Networking, syzkaller-bugs On Thu, Aug 25, 2022 at 4:34 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > Hello. Hi Tetsuo, > I found that your patch was applied. But since the reproducer tested only 0 byte > case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes > is not sufficient. > > More complete patch with Ack from Toke is waiting at > https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . Thanks for letting me know! I just checked that your patch indeed fixes the issue I am facing. If it is more complete, I think we'd indeed better use yours. > Please consider overriding with my version. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-25 15:09 ` Alexander Potapenko @ 2022-08-25 15:55 ` Toke Høiland-Jørgensen 2022-08-29 5:36 ` Kalle Valo 2022-08-26 1:35 ` Tetsuo Handa 1 sibling, 1 reply; 17+ messages in thread From: Toke Høiland-Jørgensen @ 2022-08-25 15:55 UTC (permalink / raw) To: Alexander Potapenko, Tetsuo Handa Cc: phil, ath9k-devel, David Miller, Jakub Kicinski, Kalle Valo, LKML, linux-wireless, Networking, syzkaller-bugs Alexander Potapenko <glider@google.com> writes: > On Thu, Aug 25, 2022 at 4:34 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> Hello. > Hi Tetsuo, > >> I found that your patch was applied. But since the reproducer tested only 0 byte >> case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes >> is not sufficient. >> >> More complete patch with Ack from Toke is waiting at >> https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . > > Thanks for letting me know! I just checked that your patch indeed > fixes the issue I am facing. > If it is more complete, I think we'd indeed better use yours. FWIW, that patch is just waiting for Kalle to apply it, and I just noticed this whole thread has used his old email address, so updating that now as a gentle ping :) -Toke ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-25 15:55 ` Toke Høiland-Jørgensen @ 2022-08-29 5:36 ` Kalle Valo 0 siblings, 0 replies; 17+ messages in thread From: Kalle Valo @ 2022-08-29 5:36 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Alexander Potapenko, Tetsuo Handa, phil, ath9k-devel, David Miller, Jakub Kicinski, LKML, linux-wireless, Networking, syzkaller-bugs Toke Høiland-Jørgensen <toke@toke.dk> writes: > Alexander Potapenko <glider@google.com> writes: > >> On Thu, Aug 25, 2022 at 4:34 PM Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> >>> Hello. >> Hi Tetsuo, >> >>> I found that your patch was applied. But since the reproducer tested only 0 byte >>> case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes >>> is not sufficient. >>> >>> More complete patch with Ack from Toke is waiting at >>> https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . >> >> Thanks for letting me know! I just checked that your patch indeed >> fixes the issue I am facing. >> If it is more complete, I think we'd indeed better use yours. > > FWIW, that patch is just waiting for Kalle to apply it, and I just > noticed this whole thread has used his old email address, so updating > that now as a gentle ping :) I was on vacation but back now, I'll start processing patches soon. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-25 15:09 ` Alexander Potapenko 2022-08-25 15:55 ` Toke Høiland-Jørgensen @ 2022-08-26 1:35 ` Tetsuo Handa 2022-08-26 8:38 ` Alexander Potapenko 2022-08-28 10:44 ` Phillip Potter 1 sibling, 2 replies; 17+ messages in thread From: Tetsuo Handa @ 2022-08-26 1:35 UTC (permalink / raw) To: Alexander Potapenko, ath9k-devel Cc: phil, David Miller, Jakub Kicinski, Kalle Valo, LKML, linux-wireless, Networking, syzkaller-bugs On 2022/08/26 0:09, Alexander Potapenko wrote: > On Thu, Aug 25, 2022 at 4:34 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> Hello. > Hi Tetsuo, > >> I found that your patch was applied. But since the reproducer tested only 0 byte >> case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes >> is not sufficient. >> >> More complete patch with Ack from Toke is waiting at >> https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . > > Thanks for letting me know! I just checked that your patch indeed > fixes the issue I am facing. > If it is more complete, I think we'd indeed better use yours. I recognized that "ath9k: fix an uninit value use in ath9k_htc_rx_msg()" is local to KMSAN tree. https://github.com/google/kmsan/commit/d891e35583bf2e81ccc7a2ea548bf7cf47329f40 That patch needs to be dropped, for I confirmed that passing pad_len == 8 below still triggers uninit value at ath9k_htc_fw_panic_report(). (My patch does not trigger at ath9k_htc_fw_panic_report().) fd = syz_usb_connect_ath9k(3, 0x5a, 0x20000800, 0); *(uint16_t*)0x20000880 = 0 + pad_len; *(uint16_t*)0x20000882 = 0x4e00; memmove((uint8_t*)0x20000884, "\x99\x11\x22\x33\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", 16); syz_usb_ep_write(fd, 0x82, 4 + pad_len, 0x20000880); Also, that patch has a skb leak bug; according to comment for ath9k_htc_rx_msg() * Service messages (Data, WMI) passed to the corresponding * endpoint RX handlers, which have to free the SKB. , I think that this function is supposed to free skb if skb != NULL. If dev_kfree_skb_any(skb) needs to be used when epid is invalid and pipe_id != USB_REG_IN_PIPE, why it is OK to use kfree_skb(skb) if epid == 0x99 and pipe_id != USB_REG_IN_PIPE ? We don't call kfree_skb(skb) if 0 < epid < ENDPOINT_MAX and endpoint->ep_callbacks.rx == NULL. Why it is OK not to call kfree_skb(skb) in that case? Callers can't pass such combinations? I leave these questions to ath9k developers... ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-26 1:35 ` Tetsuo Handa @ 2022-08-26 8:38 ` Alexander Potapenko 2022-08-28 10:44 ` Phillip Potter 1 sibling, 0 replies; 17+ messages in thread From: Alexander Potapenko @ 2022-08-26 8:38 UTC (permalink / raw) To: Tetsuo Handa Cc: ath9k-devel, phil, David Miller, Jakub Kicinski, Kalle Valo, LKML, linux-wireless, Networking, syzkaller-bugs On Fri, Aug 26, 2022 at 3:35 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2022/08/26 0:09, Alexander Potapenko wrote: > > On Thu, Aug 25, 2022 at 4:34 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> > >> Hello. > > Hi Tetsuo, > > > >> I found that your patch was applied. But since the reproducer tested only 0 byte > >> case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes > >> is not sufficient. > >> > >> More complete patch with Ack from Toke is waiting at > >> https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . > > > > Thanks for letting me know! I just checked that your patch indeed > > fixes the issue I am facing. > > If it is more complete, I think we'd indeed better use yours. > > I recognized that "ath9k: fix an uninit value use in ath9k_htc_rx_msg()" is > local to KMSAN tree. > https://github.com/google/kmsan/commit/d891e35583bf2e81ccc7a2ea548bf7cf47329f40 I actually did a rebase of KMSAN tree to v6.0-rc2 yesterday and dropped that patch (picked yours instead). Thanks for the heads-up! -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Liana Sebastian Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: KMSAN: uninit-value in ath9k_htc_rx_msg 2022-08-26 1:35 ` Tetsuo Handa 2022-08-26 8:38 ` Alexander Potapenko @ 2022-08-28 10:44 ` Phillip Potter 1 sibling, 0 replies; 17+ messages in thread From: Phillip Potter @ 2022-08-28 10:44 UTC (permalink / raw) To: Tetsuo Handa Cc: Alexander Potapenko, ath9k-devel, David Miller, Jakub Kicinski, Kalle Valo, LKML, linux-wireless, Networking, syzkaller-bugs On Fri, Aug 26, 2022 at 10:35:43AM +0900, Tetsuo Handa wrote: > On 2022/08/26 0:09, Alexander Potapenko wrote: > > On Thu, Aug 25, 2022 at 4:34 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> > >> Hello. > > Hi Tetsuo, > > > >> I found that your patch was applied. But since the reproducer tested only 0 byte > >> case, I think that rejecting only less than sizeof(struct htc_frame_hdr) bytes > >> is not sufficient. > >> > >> More complete patch with Ack from Toke is waiting at > >> https://lkml.kernel.org/r/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp . > > > > Thanks for letting me know! I just checked that your patch indeed > > fixes the issue I am facing. > > If it is more complete, I think we'd indeed better use yours. > > I recognized that "ath9k: fix an uninit value use in ath9k_htc_rx_msg()" is > local to KMSAN tree. > https://github.com/google/kmsan/commit/d891e35583bf2e81ccc7a2ea548bf7cf47329f40 > > That patch needs to be dropped, for I confirmed that passing pad_len == 8 below > still triggers uninit value at ath9k_htc_fw_panic_report(). (My patch does not > trigger at ath9k_htc_fw_panic_report().) > > fd = syz_usb_connect_ath9k(3, 0x5a, 0x20000800, 0); > *(uint16_t*)0x20000880 = 0 + pad_len; > *(uint16_t*)0x20000882 = 0x4e00; > memmove((uint8_t*)0x20000884, "\x99\x11\x22\x33\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", 16); > syz_usb_ep_write(fd, 0x82, 4 + pad_len, 0x20000880); > > > > Also, that patch has a skb leak bug; according to comment for ath9k_htc_rx_msg() > > * Service messages (Data, WMI) passed to the corresponding > * endpoint RX handlers, which have to free the SKB. > > , I think that this function is supposed to free skb if skb != NULL. > > If dev_kfree_skb_any(skb) needs to be used when epid is invalid and pipe_id != USB_REG_IN_PIPE, > why it is OK to use kfree_skb(skb) if epid == 0x99 and pipe_id != USB_REG_IN_PIPE ? > > We don't call kfree_skb(skb) if 0 < epid < ENDPOINT_MAX and endpoint->ep_callbacks.rx == NULL. > Why it is OK not to call kfree_skb(skb) in that case? > > Callers can't pass such combinations? I leave these questions to ath9k developers... > Hi Tetsuo, Thank you for this improved patch. My original one was somewhat naive attempt at a resolution. Regards, Phil ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2020-08-09 16:27 KMSAN: uninit-value in ath9k_htc_rx_msg syzbot 2020-08-13 3:32 ` syzbot @ 2022-07-30 12:13 ` Tetsuo Handa 2022-07-30 13:36 ` Tetsuo Handa 2022-08-16 9:52 ` Tetsuo Handa 1 sibling, 2 replies; 17+ messages in thread From: Tetsuo Handa @ 2022-07-30 12:13 UTC (permalink / raw) To: Toke Hoiland-Jorgensen, Kalle Valo; +Cc: linux-wireless, ath9k-devel syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with pkt_len = 0 but ath9k_hif_usb_rx_stream() uses __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb with uninitialized memory and ath9k_htc_rx_msg() is reading from uninitialized memory. Since bytes accessed by ath9k_htc_rx_msg() is not known until ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in ath9k_hif_usb_rx_stream(). We have two choices. One is to workaround by adding __GFP_ZERO so that ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let ath9k_htc_rx_msg() validate pkt_len before accessing. Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Since I can't find details on possible packet length used by this protocol, there might be shorter-but-valid packets. Please check carefully. drivers/net/wireless/ath/ath9k/htc_hst.c | 43 +++++++++++++++--------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c index 994ec48b2f66..ca05b07a45e6 100644 --- a/drivers/net/wireless/ath/ath9k/htc_hst.c +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c @@ -364,33 +364,27 @@ void ath9k_htc_txcompletion_cb(struct htc_target *htc_handle, } static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle, - struct sk_buff *skb) + struct sk_buff *skb, u32 len) { uint32_t *pattern = (uint32_t *)skb->data; - switch (*pattern) { - case 0x33221199: - { + if (*pattern == 0x33221199 && len >= sizeof(struct htc_panic_bad_vaddr)) { struct htc_panic_bad_vaddr *htc_panic; htc_panic = (struct htc_panic_bad_vaddr *) skb->data; dev_err(htc_handle->dev, "ath: firmware panic! " "exccause: 0x%08x; pc: 0x%08x; badvaddr: 0x%08x.\n", htc_panic->exccause, htc_panic->pc, htc_panic->badvaddr); - break; - } - case 0x33221299: - { + return; + } + if (*pattern == 0x33221299) { struct htc_panic_bad_epid *htc_panic; htc_panic = (struct htc_panic_bad_epid *) skb->data; dev_err(htc_handle->dev, "ath: firmware panic! " "bad epid: 0x%08x\n", htc_panic->epid); - break; - } - default: - dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); - break; + return; } + dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); } /* @@ -411,16 +405,26 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, if (!htc_handle || !skb) return; + /* A valid message requires len >= 8. + * + * sizeof(struct htc_frame_hdr) == 8 + * sizeof(struct htc_ready_msg) == 8 + * sizeof(struct htc_panic_bad_vaddr) == 16 + * sizeof(struct htc_panic_bad_epid) == 8 + */ + if (unlikely(len < sizeof(struct htc_frame_hdr))) + goto invalid; htc_hdr = (struct htc_frame_hdr *) skb->data; epid = htc_hdr->endpoint_id; if (epid == 0x99) { - ath9k_htc_fw_panic_report(htc_handle, skb); + ath9k_htc_fw_panic_report(htc_handle, skb, len); kfree_skb(skb); return; } if (epid < 0 || epid >= ENDPOINT_MAX) { +invalid: if (pipe_id != USB_REG_IN_PIPE) dev_kfree_skb_any(skb); else @@ -432,21 +436,30 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, /* Handle trailer */ if (htc_hdr->flags & HTC_FLAGS_RECV_TRAILER) { - if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) + if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) { /* Move past the Watchdog pattern */ htc_hdr = (struct htc_frame_hdr *)(skb->data + 4); + len -= 4; + } } /* Get the message ID */ + if (unlikely(len < sizeof(struct htc_frame_hdr) + sizeof(__be16))) + goto invalid; msg_id = (__be16 *) ((void *) htc_hdr + sizeof(struct htc_frame_hdr)); /* Now process HTC messages */ switch (be16_to_cpu(*msg_id)) { case HTC_MSG_READY_ID: + if (unlikely(len < sizeof(struct htc_ready_msg))) + goto invalid; htc_process_target_rdy(htc_handle, htc_hdr); break; case HTC_MSG_CONNECT_SERVICE_RESPONSE_ID: + if (unlikely(len < sizeof(struct htc_frame_hdr) + + sizeof(struct htc_conn_svc_rspmsg))) + goto invalid; htc_process_conn_rsp(htc_handle, htc_hdr); break; default: -- 2.34.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2022-07-30 12:13 ` [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() Tetsuo Handa @ 2022-07-30 13:36 ` Tetsuo Handa 2022-08-16 9:52 ` Tetsuo Handa 1 sibling, 0 replies; 17+ messages in thread From: Tetsuo Handa @ 2022-07-30 13:36 UTC (permalink / raw) To: Sujith Manoharan, Vasanthakumar Thiagarajan, Senthil Balasubramanian Cc: linux-wireless, ath9k-devel, Toke Hoiland-Jorgensen, Kalle Valo The "+ 32" part in __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) is there since this code was added by commit fb9987d0f748c983 ("ath9k_htc: Support for AR9271 chipset."). What is the intent of this "+ 32" ? If this is a safeguard buffer in case pkt_len was invalid, can we initialize with 0 ? On 2022/07/30 21:13, Tetsuo Handa wrote: > syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for > ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with > pkt_len = 0 but ath9k_hif_usb_rx_stream() uses > __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that > pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb > with uninitialized memory and ath9k_htc_rx_msg() is reading from > uninitialized memory. > > Since bytes accessed by ath9k_htc_rx_msg() is not known until > ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid > pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in > ath9k_hif_usb_rx_stream(). > > We have two choices. One is to workaround by adding __GFP_ZERO so that > ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let > ath9k_htc_rx_msg() validate pkt_len before accessing. > > Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] > Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > Since I can't find details on possible packet length used by this protocol, > there might be shorter-but-valid packets. Please check carefully. > > drivers/net/wireless/ath/ath9k/htc_hst.c | 43 +++++++++++++++--------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c > index 994ec48b2f66..ca05b07a45e6 100644 > --- a/drivers/net/wireless/ath/ath9k/htc_hst.c > +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c > @@ -364,33 +364,27 @@ void ath9k_htc_txcompletion_cb(struct htc_target *htc_handle, > } > > static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle, > - struct sk_buff *skb) > + struct sk_buff *skb, u32 len) > { > uint32_t *pattern = (uint32_t *)skb->data; > > - switch (*pattern) { > - case 0x33221199: > - { > + if (*pattern == 0x33221199 && len >= sizeof(struct htc_panic_bad_vaddr)) { > struct htc_panic_bad_vaddr *htc_panic; > htc_panic = (struct htc_panic_bad_vaddr *) skb->data; > dev_err(htc_handle->dev, "ath: firmware panic! " > "exccause: 0x%08x; pc: 0x%08x; badvaddr: 0x%08x.\n", > htc_panic->exccause, htc_panic->pc, > htc_panic->badvaddr); > - break; > - } > - case 0x33221299: > - { > + return; > + } > + if (*pattern == 0x33221299) { > struct htc_panic_bad_epid *htc_panic; > htc_panic = (struct htc_panic_bad_epid *) skb->data; > dev_err(htc_handle->dev, "ath: firmware panic! " > "bad epid: 0x%08x\n", htc_panic->epid); > - break; > - } > - default: > - dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); > - break; > + return; > } > + dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); > } > > /* > @@ -411,16 +405,26 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, > if (!htc_handle || !skb) > return; > > + /* A valid message requires len >= 8. > + * > + * sizeof(struct htc_frame_hdr) == 8 > + * sizeof(struct htc_ready_msg) == 8 > + * sizeof(struct htc_panic_bad_vaddr) == 16 > + * sizeof(struct htc_panic_bad_epid) == 8 > + */ > + if (unlikely(len < sizeof(struct htc_frame_hdr))) > + goto invalid; > htc_hdr = (struct htc_frame_hdr *) skb->data; > epid = htc_hdr->endpoint_id; > > if (epid == 0x99) { > - ath9k_htc_fw_panic_report(htc_handle, skb); > + ath9k_htc_fw_panic_report(htc_handle, skb, len); > kfree_skb(skb); > return; > } > > if (epid < 0 || epid >= ENDPOINT_MAX) { > +invalid: > if (pipe_id != USB_REG_IN_PIPE) > dev_kfree_skb_any(skb); > else > @@ -432,21 +436,30 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, > > /* Handle trailer */ > if (htc_hdr->flags & HTC_FLAGS_RECV_TRAILER) { > - if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) > + if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) { > /* Move past the Watchdog pattern */ > htc_hdr = (struct htc_frame_hdr *)(skb->data + 4); > + len -= 4; > + } > } > > /* Get the message ID */ > + if (unlikely(len < sizeof(struct htc_frame_hdr) + sizeof(__be16))) > + goto invalid; > msg_id = (__be16 *) ((void *) htc_hdr + > sizeof(struct htc_frame_hdr)); > > /* Now process HTC messages */ > switch (be16_to_cpu(*msg_id)) { > case HTC_MSG_READY_ID: > + if (unlikely(len < sizeof(struct htc_ready_msg))) > + goto invalid; > htc_process_target_rdy(htc_handle, htc_hdr); > break; > case HTC_MSG_CONNECT_SERVICE_RESPONSE_ID: > + if (unlikely(len < sizeof(struct htc_frame_hdr) + > + sizeof(struct htc_conn_svc_rspmsg))) > + goto invalid; > htc_process_conn_rsp(htc_handle, htc_hdr); > break; > default: ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2022-07-30 12:13 ` [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() Tetsuo Handa 2022-07-30 13:36 ` Tetsuo Handa @ 2022-08-16 9:52 ` Tetsuo Handa 2022-08-16 13:58 ` Toke Høiland-Jørgensen 1 sibling, 1 reply; 17+ messages in thread From: Tetsuo Handa @ 2022-08-16 9:52 UTC (permalink / raw) To: Toke Hoiland-Jorgensen, Kalle Valo; +Cc: linux-wireless, ath9k-devel On 2022/07/30 21:13, Tetsuo Handa wrote: > We have two choices. One is to workaround by adding __GFP_ZERO so that > ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let > ath9k_htc_rx_msg() validate pkt_len before accessing. Which choice do we want to go? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2022-08-16 9:52 ` Tetsuo Handa @ 2022-08-16 13:58 ` Toke Høiland-Jørgensen 2022-08-16 14:46 ` [PATCH v2] " Tetsuo Handa 0 siblings, 1 reply; 17+ messages in thread From: Toke Høiland-Jørgensen @ 2022-08-16 13:58 UTC (permalink / raw) To: Tetsuo Handa, Kalle Valo; +Cc: linux-wireless Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2022/07/30 21:13, Tetsuo Handa wrote: >> We have two choices. One is to workaround by adding __GFP_ZERO so that >> ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let >> ath9k_htc_rx_msg() validate pkt_len before accessing. > > Which choice do we want to go? I prefer the explicit length checks as you do in your patch. Could you please resend with an updated commit message making it explicit that this is the choice this patch is going with? -Toke ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2022-08-16 13:58 ` Toke Høiland-Jørgensen @ 2022-08-16 14:46 ` Tetsuo Handa 2022-08-16 16:38 ` Toke Høiland-Jørgensen 2022-08-30 12:16 ` Kalle Valo 0 siblings, 2 replies; 17+ messages in thread From: Tetsuo Handa @ 2022-08-16 14:46 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Kalle Valo; +Cc: linux-wireless syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with pkt_len = 0 but ath9k_hif_usb_rx_stream() uses __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb with uninitialized memory and ath9k_htc_rx_msg() is reading from uninitialized memory. Since bytes accessed by ath9k_htc_rx_msg() is not known until ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in ath9k_hif_usb_rx_stream(). We have two choices. One is to workaround by adding __GFP_ZERO so that ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let ath9k_htc_rx_msg() validate pkt_len before accessing. This patch chose the latter. Note that I'm not sure threshold condition is correct, for I can't find details on possible packet length used by this protocol. Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Changes in v2: Update patch description. drivers/net/wireless/ath/ath9k/htc_hst.c | 43 +++++++++++++++--------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c index 994ec48b2f66..ca05b07a45e6 100644 --- a/drivers/net/wireless/ath/ath9k/htc_hst.c +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c @@ -364,33 +364,27 @@ void ath9k_htc_txcompletion_cb(struct htc_target *htc_handle, } static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle, - struct sk_buff *skb) + struct sk_buff *skb, u32 len) { uint32_t *pattern = (uint32_t *)skb->data; - switch (*pattern) { - case 0x33221199: - { + if (*pattern == 0x33221199 && len >= sizeof(struct htc_panic_bad_vaddr)) { struct htc_panic_bad_vaddr *htc_panic; htc_panic = (struct htc_panic_bad_vaddr *) skb->data; dev_err(htc_handle->dev, "ath: firmware panic! " "exccause: 0x%08x; pc: 0x%08x; badvaddr: 0x%08x.\n", htc_panic->exccause, htc_panic->pc, htc_panic->badvaddr); - break; - } - case 0x33221299: - { + return; + } + if (*pattern == 0x33221299) { struct htc_panic_bad_epid *htc_panic; htc_panic = (struct htc_panic_bad_epid *) skb->data; dev_err(htc_handle->dev, "ath: firmware panic! " "bad epid: 0x%08x\n", htc_panic->epid); - break; - } - default: - dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); - break; + return; } + dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); } /* @@ -411,16 +405,26 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, if (!htc_handle || !skb) return; + /* A valid message requires len >= 8. + * + * sizeof(struct htc_frame_hdr) == 8 + * sizeof(struct htc_ready_msg) == 8 + * sizeof(struct htc_panic_bad_vaddr) == 16 + * sizeof(struct htc_panic_bad_epid) == 8 + */ + if (unlikely(len < sizeof(struct htc_frame_hdr))) + goto invalid; htc_hdr = (struct htc_frame_hdr *) skb->data; epid = htc_hdr->endpoint_id; if (epid == 0x99) { - ath9k_htc_fw_panic_report(htc_handle, skb); + ath9k_htc_fw_panic_report(htc_handle, skb, len); kfree_skb(skb); return; } if (epid < 0 || epid >= ENDPOINT_MAX) { +invalid: if (pipe_id != USB_REG_IN_PIPE) dev_kfree_skb_any(skb); else @@ -432,21 +436,30 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, /* Handle trailer */ if (htc_hdr->flags & HTC_FLAGS_RECV_TRAILER) { - if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) + if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) { /* Move past the Watchdog pattern */ htc_hdr = (struct htc_frame_hdr *)(skb->data + 4); + len -= 4; + } } /* Get the message ID */ + if (unlikely(len < sizeof(struct htc_frame_hdr) + sizeof(__be16))) + goto invalid; msg_id = (__be16 *) ((void *) htc_hdr + sizeof(struct htc_frame_hdr)); /* Now process HTC messages */ switch (be16_to_cpu(*msg_id)) { case HTC_MSG_READY_ID: + if (unlikely(len < sizeof(struct htc_ready_msg))) + goto invalid; htc_process_target_rdy(htc_handle, htc_hdr); break; case HTC_MSG_CONNECT_SERVICE_RESPONSE_ID: + if (unlikely(len < sizeof(struct htc_frame_hdr) + + sizeof(struct htc_conn_svc_rspmsg))) + goto invalid; htc_process_conn_rsp(htc_handle, htc_hdr); break; default: -- 2.18.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2022-08-16 14:46 ` [PATCH v2] " Tetsuo Handa @ 2022-08-16 16:38 ` Toke Høiland-Jørgensen 2022-08-30 12:16 ` Kalle Valo 1 sibling, 0 replies; 17+ messages in thread From: Toke Høiland-Jørgensen @ 2022-08-16 16:38 UTC (permalink / raw) To: Tetsuo Handa, Kalle Valo; +Cc: linux-wireless Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for > ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with > pkt_len = 0 but ath9k_hif_usb_rx_stream() uses > __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that > pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb > with uninitialized memory and ath9k_htc_rx_msg() is reading from > uninitialized memory. > > Since bytes accessed by ath9k_htc_rx_msg() is not known until > ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid > pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in > ath9k_hif_usb_rx_stream(). > > We have two choices. One is to workaround by adding __GFP_ZERO so that > ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let > ath9k_htc_rx_msg() validate pkt_len before accessing. This patch chose > the latter. > > Note that I'm not sure threshold condition is correct, for I can't find > details on possible packet length used by this protocol. > > Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] > Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() 2022-08-16 14:46 ` [PATCH v2] " Tetsuo Handa 2022-08-16 16:38 ` Toke Høiland-Jørgensen @ 2022-08-30 12:16 ` Kalle Valo 1 sibling, 0 replies; 17+ messages in thread From: Kalle Valo @ 2022-08-30 12:16 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Toke Høiland-Jørgensen, linux-wireless Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for > ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with > pkt_len = 0 but ath9k_hif_usb_rx_stream() uses > __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that > pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb > with uninitialized memory and ath9k_htc_rx_msg() is reading from > uninitialized memory. > > Since bytes accessed by ath9k_htc_rx_msg() is not known until > ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid > pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in > ath9k_hif_usb_rx_stream(). > > We have two choices. One is to workaround by adding __GFP_ZERO so that > ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let > ath9k_htc_rx_msg() validate pkt_len before accessing. This patch chose > the latter. > > Note that I'm not sure threshold condition is correct, for I can't find > details on possible packet length used by this protocol. > > Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] > Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. b383e8abed41 wifi: ath9k: avoid uninit memory read in ath9k_htc_rx_msg() -- https://patchwork.kernel.org/project/linux-wireless/patch/7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-08-30 12:16 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-09 16:27 KMSAN: uninit-value in ath9k_htc_rx_msg syzbot
2020-08-13 3:32 ` syzbot
[not found] ` <a142d63c-7810-40ff-9c24-7160c63bafebn@googlegroups.com>
2022-08-24 13:30 ` Alexander Potapenko
2022-08-25 14:34 ` Tetsuo Handa
2022-08-25 15:09 ` Alexander Potapenko
2022-08-25 15:55 ` Toke Høiland-Jørgensen
2022-08-29 5:36 ` Kalle Valo
2022-08-26 1:35 ` Tetsuo Handa
2022-08-26 8:38 ` Alexander Potapenko
2022-08-28 10:44 ` Phillip Potter
2022-07-30 12:13 ` [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() Tetsuo Handa
2022-07-30 13:36 ` Tetsuo Handa
2022-08-16 9:52 ` Tetsuo Handa
2022-08-16 13:58 ` Toke Høiland-Jørgensen
2022-08-16 14:46 ` [PATCH v2] " Tetsuo Handa
2022-08-16 16:38 ` Toke Høiland-Jørgensen
2022-08-30 12:16 ` Kalle Valo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.