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+031102...@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 syzk...@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

[syzbot]

syzbot has bisected this issue to:

commit dcd479e10a0510522a5d88b29b8f79ea3467d501
Author: Johannes Berg <johann...@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+031102...@syzkaller.appspotmail.com
Fixes: dcd479e10a05 ("mac80211: always wind down STA state")

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

[Pavel Skripkin]

Hi, everybody!

I spend some time debugging this bug and I want to share the

results, maybe, someone will fix it in proper way (I really do not 

understand how to do it)

So, it's not truly 'use-after-free': the problem is in invalid 

memcpy length.

#define MAX_RX_BUF_SIZE 16384

static int ath9k_hif_usb_alloc_rx_urbs(struct hif_device_usb *hif_dev)

{

.... 

         skb = alloc_skb(MAX_RX_BUF_SIZE, GFP_KERNEL);

.... 

}

but in ath9k_hif_usb_rx_stream()

static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,

struct sk_buff *skb)

{

...

          memcpy(ptr, skb->data, rx_remain_len);

...

}

rx_remain_len can be greater then MAX_RX_BUF_SIZE: 

if (index > MAX_RX_BUF_SIZE) {

...

         hif_dev->rx_remain_len = index - MAX_RX_BUF_SIZE;

This 'if' doesn't cover case, when index > MAX_RX_BUF_SIZE * 2.

That's all I know about this issue, I hope I haven't wasted of my time :)

Good luck!

[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+031102...@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+031102...@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

[Pavel Skripkin]

Hi Tetsuo,

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/80962aae265995d1cdb724f5362c556d4...@gmail.com/



With regards,
Pavel Skripkin

[Tetsuo Handa]

For unknown reason, "Last activity" field for this bug is updating without visible changes.
In case somebody is trying something, a patch for this bug is waiting for Toke to test.
( https://lore.kernel.org/all/87o804w...@toke.dk/ )

# I wish that syzbot dashboard allows manually specifying e.g. "patch proposed" state, for
# some patches take long time before accepted (and may result in duplicated works like I did
# on this bug).

[Dmitry Vyukov]

+Aleksandr for /\/\/\/\/\/\/\/\/\/\

[Pavel Skripkin]

Hi Tetsuo,

On 6/15/22 13:56, Tetsuo Handa wrote:
> For unknown reason, "Last activity" field for this bug is updating without visible changes.
> In case somebody is trying something, a patch for this bug is waiting for Toke to test.
> ( https://lore.kernel.org/all/87o804w...@toke.dk/ )
>

Reasons are clear: there is a discussion on v6 of that patch [1].

> # I wish that syzbot dashboard allows manually specifying e.g. "patch proposed" state, for
> # some patches take long time before accepted (and may result in duplicated works like I did
> # on this bug).
>

That's a good point!


[1]
https://lore.kernel.org/linux-wireless/d57bbedc857950659bfacac0ab48790c1...@gmail.com/




With regards,
Pavel Skripkin

[Tetsuo Handa]

On 2022/06/15 21:04, Pavel Skripkin wrote:
> Hi Tetsuo,
>
> On 6/15/22 13:56, Tetsuo Handa wrote:
>> For unknown reason, "Last activity" field for this bug is updating without visible changes.
>> In case somebody is trying something, a patch for this bug is waiting for Toke to test.
>> ( https://lore.kernel.org/all/87o804w...@toke.dk/ )
>>
>
> Reasons are clear: there is a discussion on v6 of that patch [1].
>

I see. Then, the reason no visible change was made to
https://groups.google.com/g/syzkaller-bugs/c/ysjnCmKu6Dc/m/027R0aA2DAAJ while
only "Last activity" was updated is that v6 patch was submitted without
syzkall...@googlegroups.com , syzkaller-bugs refused to add the v6 patch to
that page?

I expected that sending to the address in Reported-by: field adds to that page...

[Aleksandr Nogikh]

Hi Tetsuo,

On Wed, Jun 15, 2022 at 1:06 PM Dmitry Vyukov <dvy...@google.com> wrote:
>
> On Wed, 15 Jun 2022 at 12:56, Tetsuo Handa
> <penguin...@i-love.sakura.ne.jp> wrote:
> >
> > For unknown reason, "Last activity" field for this bug is updating without visible changes.
> > In case somebody is trying something, a patch for this bug is waiting for Toke to test.
> > ( https://lore.kernel.org/all/87o804w...@toke.dk/ )

What exactly did you mean by "visible changes" in this case?
FTR we have this issue: https://github.com/google/syzkaller/issues/1574

> >
> > # I wish that syzbot dashboard allows manually specifying e.g. "patch proposed" state, for
> > # some patches take long time before accepted (and may result in duplicated works like I did
> > # on this bug).

Thanks for the suggestion!
I have noted this feature request in
https://github.com/google/syzkaller/issues/1978, I hope we'll get to
implementing that in some foreseeable future.

>
> +Aleksandr for /\/\/\/\/\/\/\/\/\/\

Best Regards,
Aleksandr

[Tetsuo Handa]

On 2022/06/20 20:14, Aleksandr Nogikh wrote:
>>> For unknown reason, "Last activity" field for this bug is updating without visible changes.
>>> In case somebody is trying something, a patch for this bug is waiting for Toke to test.
>>> ( https://lore.kernel.org/all/87o804w...@toke.dk/ )
>
> What exactly did you mean by "visible changes" in this case?
> FTR we have this issue: https://github.com/google/syzkaller/issues/1574

Nothing changed in https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60
and https://groups.google.com/d/msgid/syzkaller-bugs/00000000000055348705b43c701d%40google.com
when "Last activity" field for this bug in https://syzkaller.appspot.com/upstream page changed.
I couldn't find the fact that a new patch was proposed to ML.

>>> # I wish that syzbot dashboard allows manually specifying e.g. "patch proposed" state, for
>>> # some patches take long time before accepted (and may result in duplicated works like I did
>>> # on this bug).
>
> Thanks for the suggestion!
> I have noted this feature request in
> https://github.com/google/syzkaller/issues/1978, I hope we'll get to
> implementing that in some foreseeable future.

An easy way would be to accept syzbot command like

#syz proposed-patch: https://lore.kernel.org/all/80962aae265995d1cdb724f5362c556d4...@gmail.com/

or

#syz discussion: https://lore.kernel.org/all/80962aae265995d1cdb724f5362c556d4...@gmail.com/

and show that link at

Fix commit:

line of https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 page.

When a new version of patch is submitted, issuing this command again overwrites the link.
Since this command is simply for recording a link, this command could be used for recording
where the current discussion is as well as where the latest patch is.

When a patch with Reported-by: line arrived at one of git trees syzbot is monitoring,
this link is replaced with current behavior.

[Aleksandr Nogikh]

Hi Tetsuo,

On Mon, Jun 20, 2022 at 1:37 PM Tetsuo Handa
<penguin...@i-love.sakura.ne.jp> wrote:
>
> On 2022/06/20 20:14, Aleksandr Nogikh wrote:
> >>> For unknown reason, "Last activity" field for this bug is updating without visible changes.
> >>> In case somebody is trying something, a patch for this bug is waiting for Toke to test.
> >>> ( https://lore.kernel.org/all/87o804w...@toke.dk/ )
> >
> > What exactly did you mean by "visible changes" in this case?
> > FTR we have this issue: https://github.com/google/syzkaller/issues/1574
>
> Nothing changed in https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60
> and https://groups.google.com/d/msgid/syzkaller-bugs/00000000000055348705b43c701d%40google.com
> when "Last activity" field for this bug in https://syzkaller.appspot.com/upstream page changed.
> I couldn't find the fact that a new patch was proposed to ML.

As I understand (Dmitry, please correct me if I'm wrong), syzbot will
update the Last activity field every time it receives a bug-related
email, even if there was no command to the bot itself. In this case,
it could have indeed been the emails in the discussion you've
mentioned in your other email. Note that
https://lore.kernel.org/linux-wireless/165571934517.23287.236...@kernel.org/
cc's two syzbo...@syzkaller.appspotmail.com emails.

I'm thinking now that it would be indeed much less confusing if syzbot
also remembered the exact links to the lkml messages that have cc'd
the bug.

>
> >>> # I wish that syzbot dashboard allows manually specifying e.g. "patch proposed" state, for
> >>> # some patches take long time before accepted (and may result in duplicated works like I did
> >>> # on this bug).
> >
> > Thanks for the suggestion!
> > I have noted this feature request in
> > https://github.com/google/syzkaller/issues/1978, I hope we'll get to
> > implementing that in some foreseeable future.
>
> An easy way would be to accept syzbot command like
>
> #syz proposed-patch: https://lore.kernel.org/all/80962aae265995d1cdb724f5362c556d4...@gmail.com/
>
> or
>
> #syz discussion: https://lore.kernel.org/all/80962aae265995d1cdb724f5362c556d4...@gmail.com/
>
> and show that link at
>
> Fix commit:
>
> line of https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 page.
>
> When a new version of patch is submitted, issuing this command again overwrites the link.
> Since this command is simply for recording a link, this command could be used for recording
> where the current discussion is as well as where the latest patch is.

That looks reasonable.