KMSAN: uninit-value in ath9k_htc_rx_msg

KMSAN: uninit-value in ath9k_htc_rx_msg

[syzbot]

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.

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[syzbot]

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
=====================================================

[PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Tetsuo Handa]

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

Re: [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Tetsuo Handa]

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:

Re: [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Tetsuo Handa]

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?

Re: [PATCH] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Toke Høiland-Jørgensen]

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

[PATCH v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Tetsuo Handa]

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

Re: [PATCH v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Toke Høiland-Jørgensen]

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>

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Alexander Potapenko]

(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

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Tetsuo Handa]

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;
>> ==============================================================
> 

> 

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Alexander Potapenko]

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.

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Toke Høiland-Jørgensen]

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

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Tetsuo Handa]

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...

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Alexander Potapenko]

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

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Phillip Potter]

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

Re: KMSAN: uninit-value in ath9k_htc_rx_msg

[Kalle Valo]

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

Re: [PATCH v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

[Kalle Valo]

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