KASAN: use-after-free Read in ath9k_hif_usb_rx_cb (2)

KASAN: use-after-free Read in ath9k_hif_usb_rx_cb (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    0fb2c41f Merge 5.10-rc4 into here.
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
console output: https://syzkaller.appspot.com/x/log.txt?x=15746da1500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b99cde3c953e8f6e
dashboard link: https://syzkaller.appspot.com/bug?extid=03110230a11411024147
compiler:       gcc (GCC) 10.1.0-syz 20200507
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12cc9bba500000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=120b1cc2500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: use-after-free in memcpy include/linux/string.h:399 [inline]
BUG: KASAN: use-after-free in ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:562 [inline]
BUG: KASAN: use-after-free in ath9k_hif_usb_rx_cb+0x3ab/0x1020 drivers/net/wireless/ath/ath9k/hif_usb.c:680
Read of size 49146 at addr ffff888113938000 by task swapper/1/0

CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.10.0-rc4-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+0x107/0x163 lib/dump_stack.c:118
 print_address_description.constprop.0.cold+0xae/0x4c8 mm/kasan/report.c:385
 __kasan_report mm/kasan/report.c:545 [inline]
 kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
 check_memory_region_inline mm/kasan/generic.c:186 [inline]
 check_memory_region+0x13d/0x180 mm/kasan/generic.c:192
 memcpy+0x20/0x60 mm/kasan/common.c:105
 memcpy include/linux/string.h:399 [inline]
 ath9k_hif_usb_rx_stream drivers/net/wireless/ath/ath9k/hif_usb.c:562 [inline]
 ath9k_hif_usb_rx_cb+0x3ab/0x1020 drivers/net/wireless/ath/ath9k/hif_usb.c:680
 __usb_hcd_giveback_urb+0x2b0/0x5c0 drivers/usb/core/hcd.c:1657
 usb_hcd_giveback_urb+0x367/0x410 drivers/usb/core/hcd.c:1728
 dummy_timer+0x11f4/0x3280 drivers/usb/gadget/udc/dummy_hcd.c:1969
 call_timer_fn+0x1a5/0x630 kernel/time/timer.c:1410
 expire_timers kernel/time/timer.c:1455 [inline]
 __run_timers.part.0+0x67c/0xa10 kernel/time/timer.c:1747
 __run_timers kernel/time/timer.c:1728 [inline]
 run_timer_softirq+0x80/0x120 kernel/time/timer.c:1760
 __do_softirq+0x1b2/0x945 kernel/softirq.c:298
 asm_call_irq_on_stack+0xf/0x20
 </IRQ>
 __run_on_irqstack arch/x86/include/asm/irq_stack.h:26 [inline]
 run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:77 [inline]
 do_softirq_own_stack+0x80/0xa0 arch/x86/kernel/irq_64.c:77
 invoke_softirq kernel/softirq.c:393 [inline]
 __irq_exit_rcu kernel/softirq.c:423 [inline]
 irq_exit_rcu+0x110/0x1a0 kernel/softirq.c:435
 sysvec_apic_timer_interrupt+0x43/0xa0 arch/x86/kernel/apic/apic.c:1091
 asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:631
RIP: 0010:native_save_fl arch/x86/include/asm/irqflags.h:29 [inline]
RIP: 0010:arch_local_save_flags arch/x86/include/asm/irqflags.h:79 [inline]
RIP: 0010:arch_irqs_disabled arch/x86/include/asm/irqflags.h:169 [inline]
RIP: 0010:acpi_safe_halt drivers/acpi/processor_idle.c:112 [inline]
RIP: 0010:acpi_idle_do_entry+0x1c9/0x250 drivers/acpi/processor_idle.c:517
Code: 8d d1 a1 fb 84 db 75 ac e8 34 d9 a1 fb e8 df 7f a7 fb e9 0c 00 00 00 e8 25 d9 a1 fb 0f 00 2d 6e 7b 6a 00 e8 19 d9 a1 fb fb f4 <9c> 5b 81 e3 00 02 00 00 fa 31 ff 48 89 de e8 b4 d1 a1 fb 48 85 db
RSP: 0018:ffffc900000dfd18 EFLAGS: 00000293
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 1ffffffff10796c9
RDX: ffff888100293280 RSI: ffffffff859d0937 RDI: ffffffff859d0921
RBP: ffff8881031b7864 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
R13: ffff8881031b7800 R14: ffff8881031b7864 R15: ffff888105aee804
 acpi_idle_enter+0x355/0x4f0 drivers/acpi/processor_idle.c:648
 cpuidle_enter_state+0x1b1/0xc80 drivers/cpuidle/cpuidle.c:237
 cpuidle_enter+0x4a/0xa0 drivers/cpuidle/cpuidle.c:351
 call_cpuidle kernel/sched/idle.c:132 [inline]
 cpuidle_idle_call kernel/sched/idle.c:213 [inline]
 do_idle+0x3d5/0x580 kernel/sched/idle.c:273
 cpu_startup_entry+0x14/0x20 kernel/sched/idle.c:369
 start_secondary+0x265/0x340 arch/x86/kernel/smpboot.c:266
 secondary_startup_64_no_verify+0xb0/0xbb

The buggy address belongs to the page:
page:00000000d417cdb1 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x113938
head:00000000d417cdb1 order:3 compound_mapcount:0 compound_pincount:0
flags: 0x200000000010000(head)
raw: 0200000000010000 dead000000000100 dead000000000122 0000000000000000
raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88811393ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff88811393ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff888113940000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                   ^
 ffff888113940080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888113940100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
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.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: KASAN: use-after-free Read in ath9k_hif_usb_rx_cb (2)

[syzbot]

syzbot has bisected this issue to:

commit dcd479e10a0510522a5d88b29b8f79ea3467d501
Author: Johannes Berg <johannes.berg@intel.com>
Date:   Fri Oct 9 12:17:11 2020 +0000

    mac80211: always wind down STA state

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=100c9c16500000
start commit:   0fa8ee0d Merge branch 'for-linus' of git://git.kernel.org/..
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=120c9c16500000
console output: https://syzkaller.appspot.com/x/log.txt?x=140c9c16500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=75292221eb79ace2
dashboard link: https://syzkaller.appspot.com/bug?extid=03110230a11411024147
userspace arch: i386
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1587f841500000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=11ec0fe6500000

Reported-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
Fixes: dcd479e10a05 ("mac80211: always wind down STA state")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[PATCH] ath9k: fix use-after-free read at ath9k_hif_usb_rx_cb()

[Tetsuo Handa]

Although hif_dev->htc_handle is allocated by ath9k_htc_hw_alloc() from
ath9k_hif_usb_firmware_cb(), hif_dev->htc_handle->drv_priv is not assigned
until ieee80211_alloc_hw() from ath9k_htc_probe_device() from
ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() returns. However, as
soon as ath9k_hif_usb_alloc_rx_urbs() from ath9k_hif_usb_alloc_urbs() from
ath9k_hif_usb_dev_init() from ath9k_hif_usb_firmware_cb() returns, a timer
interrupt can access hif_dev->htc_handle->drv_priv via RX_STAT_INC() from
ath9k_hif_usb_rx_stream() from ath9k_hif_usb_rx_cb() from
usb_hcd_giveback_urb(), which results in NULL pointer deference problem.

Also, even after htc_handle->drv_priv is assigned, when
ath9k_htc_wait_for_target() from ath9k_htc_probe_device() from
ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() fails,
ieee80211_free_hw() (which does not reset hif_dev->htc_handle->drv_priv)
is immediately called due to "goto err_free;". As a result, a timer
interrupt which happens after ieee80211_free_hw() triggers use-after-free
problem at the abovementioned location.

We can flush in-flight requests by calling ath9k_hif_usb_dealloc_urbs()
before calling ieee80211_free_hw(). But we need to take from two choices
for not yet assigned case. One is to change e.g. RX_STAT_INC() to check
for NULL because it depends on CONFIG_ATH9K_HTC_DEBUGFS=y. The other is to
assign a dummy object which is used until initialization. This patch took
the latter.

Link: https://syzkaller.appspot.com/bug?extid=03110230a11411024147
Reported-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
---
Pavel Skripkin has tested "check for NULL" approach, but not yet accepted.
What was wrong with Pavel's approach?

 drivers/net/wireless/ath/ath9k/htc_drv_init.c | 6 +++---
 drivers/net/wireless/ath/ath9k/htc_hst.c      | 5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index ff61ae34ecdf..e497a44aff88 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -931,7 +931,6 @@ static int ath9k_init_device(struct ath9k_htc_priv *priv,
 int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 			   u16 devid, char *product, u32 drv_info)
 {
-	struct hif_device_usb *hif_dev;
 	struct ath9k_htc_priv *priv;
 	struct ieee80211_hw *hw;
 	int ret;
@@ -969,10 +968,11 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 
 err_init:
 	ath9k_stop_wmi(priv);
-	hif_dev = (struct hif_device_usb *)htc_handle->hif_dev;
-	ath9k_hif_usb_dealloc_urbs(hif_dev);
+	ath9k_hif_usb_dealloc_urbs((struct hif_device_usb *)htc_handle->hif_dev);
 	ath9k_destroy_wmi(priv);
 err_free:
+	ath9k_hif_usb_dealloc_urbs((struct hif_device_usb *)htc_handle->hif_dev);
+	htc_handle->drv_priv = NULL;
 	ieee80211_free_hw(hw);
 	return ret;
 }
diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index 994ec48b2f66..d461eca389ab 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -468,6 +468,9 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
 	}
 }
 
+/* A dummy object used until device is initialized. */
+static struct ath9k_htc_priv ath9k_uninitialized_htc_priv;
+
 struct htc_target *ath9k_htc_hw_alloc(void *hif_handle,
 				      struct ath9k_htc_hif *hif,
 				      struct device *dev)
@@ -493,6 +496,8 @@ struct htc_target *ath9k_htc_hw_alloc(void *hif_handle,
 
 	atomic_set(&target->tgt_ready, 0);
 
+	target->drv_priv = &ath9k_uninitialized_htc_priv;
+
 	return target;
 }
 
-- 
2.34.1

Re: [PATCH] ath9k: fix use-after-free read at ath9k_hif_usb_rx_cb()

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 2378 bytes --]

Hi Tetsuo,

On 4/29/22 14:18, Tetsuo Handa wrote:
> Although hif_dev->htc_handle is allocated by ath9k_htc_hw_alloc() from
> ath9k_hif_usb_firmware_cb(), hif_dev->htc_handle->drv_priv is not assigned
> until ieee80211_alloc_hw() from ath9k_htc_probe_device() from
> ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() returns. However, as
> soon as ath9k_hif_usb_alloc_rx_urbs() from ath9k_hif_usb_alloc_urbs() from
> ath9k_hif_usb_dev_init() from ath9k_hif_usb_firmware_cb() returns, a timer
> interrupt can access hif_dev->htc_handle->drv_priv via RX_STAT_INC() from
> ath9k_hif_usb_rx_stream() from ath9k_hif_usb_rx_cb() from
> usb_hcd_giveback_urb(), which results in NULL pointer deference problem.
> 
> Also, even after htc_handle->drv_priv is assigned, when
> ath9k_htc_wait_for_target() from ath9k_htc_probe_device() from
> ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() fails,
> ieee80211_free_hw() (which does not reset hif_dev->htc_handle->drv_priv)
> is immediately called due to "goto err_free;". As a result, a timer
> interrupt which happens after ieee80211_free_hw() triggers use-after-free
> problem at the abovementioned location.
> 
> We can flush in-flight requests by calling ath9k_hif_usb_dealloc_urbs()
> before calling ieee80211_free_hw(). But we need to take from two choices
> for not yet assigned case. One is to change e.g. RX_STAT_INC() to check
> for NULL because it depends on CONFIG_ATH9K_HTC_DEBUGFS=y. The other is to
> assign a dummy object which is used until initialization. This patch took
> the latter.
> 
> Link: https://syzkaller.appspot.com/bug?extid=03110230a11411024147
> Reported-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
> ---
> Pavel Skripkin has tested "check for NULL" approach, but not yet accepted.
> What was wrong with Pavel's approach?
> 

I don't know. IIRC the problem is that nobody has tested my patch on 
real hw, so they can't accept it as-is. And maybe it just got lost

You can check out [1] thread. It's the latest version I have posted



[1] 
https://lore.kernel.org/all/80962aae265995d1cdb724f5362c556d494c7566.1644265120.git.paskripkin@gmail.com/



With regards,
Pavel Skripkin




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]