WARNING in wdm_write/usb_submit_urb

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: d0847550 usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=16462bba600000
kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19
dashboard link: https://syzkaller.appspot.com/bug?extid=d232cca6ec42c2edb3fc
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15fac696600000

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

------------[ cut here ]------------
URB 00000000860a3d7f submitted while active
WARNING: CPU: 0 PID: 2801 at drivers/usb/core/urb.c:362
usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 2801 Comm: syz-executor.5 Not tainted 5.3.0-rc4+ #26
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113
panic+0x2a3/0x6da kernel/panic.c:219
__warn.cold+0x20/0x4a kernel/panic.c:576
report_bug+0x262/0x2a0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
RIP: 0010:usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
Code: 89 de e8 62 b7 ef fd 84 db 0f 85 42 f6 ff ff e8 25 b6 ef fd 4c 89 fe
48 c7 c7 c0 67 18 86 c6 05 07 29 3a 04 01 e8 14 9b c5 fd <0f> 0b e9 20 f6
ff ff c7 44 24 14 01 00 00 00 e9 d7 f6 ff ff 41 bd
RSP: 0018:ffff8881d9647c98 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81288cfd RDI: ffffed103b2c8f85
RBP: ffff8881ca339220 R08: ffff8881d96bb000 R09: ffffed103b645d58
R10: ffffed103b645d57 R11: ffff8881db22eabf R12: 1ffff1103b2c8fa7
R13: 00000000fffffff0 R14: ffff8881ca339208 R15: ffff8881c8b53900
wdm_write+0x828/0xd87 drivers/usb/class/cdc-wdm.c:423
__vfs_write+0x76/0x100 fs/read_write.c:494
vfs_write+0x262/0x5c0 fs/read_write.c:558
ksys_write+0x127/0x250 fs/read_write.c:611
do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x459829
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f8693ebdc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000459829
RDX: 0000000000000020 RSI: 0000000020000040 RDI: 0000000000000004
RBP: 000000000075c070 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8693ebe6d4
R13: 00000000004c99ec R14: 00000000004e1088 R15: 00000000ffffffff
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug 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 bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[syzbot]

syzbot has found a reproducer for the following crash on:

HEAD commit: e06ce4da usb-fuzzer: main usb gadget fuzzer driver

git tree: https://github.com/google/kasan.git usb-fuzzer

console output: https://syzkaller.appspot.com/x/log.txt?x=14a8c0b6600000
kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e

dashboard link: https://syzkaller.appspot.com/bug?extid=d232cca6ec42c2edb3fc
compiler: gcc (GCC) 9.0.0 20181231 (experimental)

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12b6dfba600000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15f63a4c600000

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

------------[ cut here ]------------

URB 000000005fab893a submitted while active
WARNING: CPU: 1 PID: 1788 at drivers/usb/core/urb.c:362

usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
Kernel panic - not syncing: panic_on_warn set ...

CPU: 1 PID: 1788 Comm: syz-executor522 Not tainted 5.3.0-rc5+ #27

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113
panic+0x2a3/0x6da kernel/panic.c:219
__warn.cold+0x20/0x4a kernel/panic.c:576
report_bug+0x262/0x2a0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
RIP: 0010:usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362

Code: 89 de e8 82 bc ef fd 84 db 0f 85 42 f6 ff ff e8 45 bb ef fd 4c 89 fe
48 c7 c7 80 68 18 86 c6 05 27 30 3a 04 01 e8 34 a1 c5 fd <0f> 0b e9 20 f6

ff ff c7 44 24 14 01 00 00 00 e9 d7 f6 ff ff 41 bd

RSP: 0018:ffff8881d036fc98 EFLAGS: 00010286

RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000

RDX: 0000000000000000 RSI: ffffffff81288cfd RDI: ffffed103a06df85
RBP: ffff8881cfce56a0 R08: ffff8881d1ce4800 R09: ffffed103b663ee7
R10: ffffed103b663ee6 R11: ffff8881db31f737 R12: 1ffff1103a06dfa7
R13: 00000000fffffff0 R14: ffff8881cfce5688 R15: ffff8881d8106d00

wdm_write+0x828/0xd87 drivers/usb/class/cdc-wdm.c:423
__vfs_write+0x76/0x100 fs/read_write.c:494
vfs_write+0x262/0x5c0 fs/read_write.c:558
ksys_write+0x127/0x250 fs/read_write.c:611
do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe

RIP: 0033:0x447029
Code: e8 ec e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7

48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff

ff 0f 83 3b 07 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f1e9e0a4da8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00000000006dcc28 RCX: 0000000000447029
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000004
RBP: 00000000006dcc20 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006dcc2c
R13: 0000000020000000 R14: 00000000004af170 R15: 00000000000003e8

[Oliver Neukum]

Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:

Hi Bjørn,

taking you into CC as you are affected.

#syz test: https://github.com/google/kasan.git e06ce4da

From 38dda97aa4820a60c680374edbab0323cfcb0193 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <one...@suse.com>
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse

In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.

The issue is as old as the driver.

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cc...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <one...@suse.com>
---
drivers/usb/class/cdc-wdm.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..a341081a5f47 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;

- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+ wait_event(desc->wait,
+ /*
+ * needs both flags. We cannot do with one
+ * because resetting it would cause a race
+ * with write() yet we need to signal
+ * a disconnect
+ */
+ !test_bit(WDM_IN_USE, &desc->flags) &&
+ !test_bit(WDM_DISCONNECTING, &desc->flags));

/* cannot dereference desc->intf if WDM_DISCONNECTING */
if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
dev_err(&desc->intf->dev, "Error in flush path: %d\n",
desc->werr);

- return usb_translate_errors(desc->werr);
+ return test_bit(WDM_DISCONNECTING, &desc->flags) ?
+ -ENODEV :
+ usb_translate_errors(desc->werr);
}

static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
@@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
spin_lock_irqsave(&desc->iuspin, flags);
set_bit(WDM_DISCONNECTING, &desc->flags);
set_bit(WDM_READ, &desc->flags);
- /* to terminate pending flushes */
- clear_bit(WDM_IN_USE, &desc->flags);
spin_unlock_irqrestore(&desc->iuspin, flags);
wake_up_all(&desc->wait);
mutex_lock(&desc->rlock);
--
2.16.4

[Bjørn Mork]

Oliver Neukum <one...@suse.com> writes:

> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 1656f5155ab8..a341081a5f47 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
> {
> struct wdm_device *desc = file->private_data;
>
> - wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> + wait_event(desc->wait,
> + /*
> + * needs both flags. We cannot do with one
> + * because resetting it would cause a race
> + * with write() yet we need to signal
> + * a disconnect
> + */
> + !test_bit(WDM_IN_USE, &desc->flags) &&
> + !test_bit(WDM_DISCONNECTING, &desc->flags));

Makes sense. But isn't the WDM_DISCONNECTING test inverted?

> /* cannot dereference desc->intf if WDM_DISCONNECTING */
> if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
> dev_err(&desc->intf->dev, "Error in flush path: %d\n",
> desc->werr);
>
> - return usb_translate_errors(desc->werr);
> + return test_bit(WDM_DISCONNECTING, &desc->flags) ?
> + -ENODEV :
> + usb_translate_errors(desc->werr);
> }

Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
here. How about

if (test_bit(WDM_DISCONNECTING, &desc->flags))
return -ENODEV;
if (desc->werr < 0)

dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);

return usb_translate_errors(desc->werr);

> static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
> @@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
> spin_lock_irqsave(&desc->iuspin, flags);
> set_bit(WDM_DISCONNECTING, &desc->flags);
> set_bit(WDM_READ, &desc->flags);
> - /* to terminate pending flushes */
> - clear_bit(WDM_IN_USE, &desc->flags);
> spin_unlock_irqrestore(&desc->iuspin, flags);
> wake_up_all(&desc->wait);
> mutex_lock(&desc->rlock);

Yes, this looks much better.


Bjørn

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer still triggered
crash:
INFO: task hung in wdm_flush

INFO: task syz-executor.1:2841 blocked for more than 143 seconds.
Not tainted 5.3.0-rc5+ #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
syz-executor.1 D28520 2841 1862 0x00000004
Call Trace:
schedule+0x9a/0x250 kernel/sched/core.c:3944
wdm_flush+0x18a/0x460 drivers/usb/class/cdc-wdm.c:590
filp_close+0xb4/0x160 fs/open.c:1166
__close_fd+0x133/0x200 fs/file.c:636
__do_sys_close fs/open.c:1185 [inline]
__se_sys_close fs/open.c:1183 [inline]
__x64_sys_close+0x69/0x100 fs/open.c:1183
do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x413511
Code: 48 8b 40 30 83 80 00 01 00 00 ff 48 8b 6c 24 20 48 83 c4 28 c3 48 8d
05 07 31 24 00 48 89 04 24 48 c7 44 24 08 16 00 00 00 e8 <1b> 57 01 00 0f
0b 48 8d 05 15 00 22 00 48 89 04 24 48 c7 44 24 08
RSP: 002b:00007ffc1111a550 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 0000000000413511
RDX: fffffffffffffff7 RSI: 0000000000000000 RDI: 0000000000000004
RBP: 0000000000000001 R08: ffffffffffffffff R09: ffffffffffffffff
R10: 00007ffc1111a630 R11: 0000000000000293 R12: 000000000075bf20
R13: 000000000000a6c3 R14: 0000000000760488 R15: ffffffffffffffff
INFO: task syz-executor.5:2855 blocked for more than 143 seconds.
Not tainted 5.3.0-rc5+ #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
syz-executor.5 D28168 2855 1871 0x00000004
Call Trace:
schedule+0x9a/0x250 kernel/sched/core.c:3944
wdm_flush+0x18a/0x460 drivers/usb/class/cdc-wdm.c:590
filp_close+0xb4/0x160 fs/open.c:1166
__close_fd+0x133/0x200 fs/file.c:636
__do_sys_close fs/open.c:1185 [inline]
__se_sys_close fs/open.c:1183 [inline]
__x64_sys_close+0x69/0x100 fs/open.c:1183
do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x413511
Code: 48 8b 40 30 83 80 00 01 00 00 ff 48 8b 6c 24 20 48 83 c4 28 c3 48 8d
05 07 31 24 00 48 89 04 24 48 c7 44 24 08 16 00 00 00 e8 <1b> 57 01 00 0f
0b 48 8d 05 15 00 22 00 48 89 04 24 48 c7 44 24 08
RSP: 002b:00007fff97397960 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 0000000000413511
RDX: fffffffffffffff7 RSI: 0000000000000000 RDI: 0000000000000004
RBP: 0000000000000001 R08: ffffffffffffffff R09: ffffffffffffffff
R10: 00007fff97397a40 R11: 0000000000000293 R12: 000000000075bf20
R13: 000000000000a894 R14: 0000000000760488 R15: ffffffffffffffff

Showing all locks held in the system:
1 lock held by khungtaskd/23:
#0: 0000000084b787af (rcu_read_lock){....}, at:
debug_show_all_locks+0x53/0x269 kernel/locking/lockdep.c:5254
1 lock held by rsyslogd/1604:
2 locks held by getty/1694:
#0: 000000001a489867 (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 00000000a1f14aef (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156
2 locks held by getty/1695:
#0: 00000000b6d2253c (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 000000006c54f9ae (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156
2 locks held by getty/1696:
#0: 00000000393fdbe8 (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 00000000946abfea (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156
2 locks held by getty/1697:
#0: 00000000b4d6d459 (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 00000000b782f4b1 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156
2 locks held by getty/1698:
#0: 000000005059df5d (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 000000009805da5f (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156
2 locks held by getty/1699:
#0: 00000000bed26f3a (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 000000004ed38206 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156
2 locks held by getty/1700:
#0: 0000000040122501 (&tty->ldisc_sem){++++}, at:
tty_ldisc_ref_wait+0x22/0x80 drivers/tty/tty_ldisc.c:272
#1: 00000000ddb347fb (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x223/0x1ae0 drivers/tty/n_tty.c:2156

=============================================

NMI backtrace for cpu 1
CPU: 1 PID: 23 Comm: khungtaskd Not tainted 5.3.0-rc5+ #1

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113

nmi_cpu_backtrace.cold+0x55/0x96 lib/nmi_backtrace.c:101
nmi_trigger_cpumask_backtrace+0x1b0/0x1c7 lib/nmi_backtrace.c:62
trigger_all_cpu_backtrace include/linux/nmi.h:146 [inline]
check_hung_uninterruptible_tasks kernel/hung_task.c:205 [inline]
watchdog+0x9a4/0xe50 kernel/hung_task.c:289
kthread+0x318/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Sending NMI from CPU 1 to CPUs 0:
NMI backtrace for cpu 0 skipped: idling at native_safe_halt
arch/x86/include/asm/irqflags.h:60 [inline]
NMI backtrace for cpu 0 skipped: idling at arch_safe_halt
arch/x86/include/asm/irqflags.h:103 [inline]
NMI backtrace for cpu 0 skipped: idling at default_idle+0x28/0x2e0
arch/x86/kernel/process.c:580


Tested on:

commit: e06ce4da usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git

console output: https://syzkaller.appspot.com/x/log.txt?x=1767eb4a600000
kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e

compiler: gcc (GCC) 9.0.0 20181231 (experimental)

patch: https://syzkaller.appspot.com/x/patch.diff?x=1667eb4a600000

[Oliver Neukum]

Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:

> syzbot has found a reproducer for the following crash on:

Hi Bjørn,

taking you into CC as you are affected.

V2: fix logic bug

Regards
Oliver

#syz test: https://github.com/google/kasan.git e06ce4da

From 8d0c7a38b1a4883e7bdab76b06a9e564fc466506 Mon Sep 17 00:00:00 2001

From: Oliver Neukum <one...@suse.com>
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse

In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.

The issue is as old as the driver.

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cc...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <one...@suse.com>
---
drivers/usb/class/cdc-wdm.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..2b341f3a155f 100644

--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;

- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+ wait_event(desc->wait,
+ /*
+ * needs both flags. We cannot do with one
+ * because resetting it would cause a race
+ * with write() yet we need to signal
+ * a disconnect
+ */
+ !test_bit(WDM_IN_USE, &desc->flags) ||

+ !test_bit(WDM_DISCONNECTING, &desc->flags));

/* cannot dereference desc->intf if WDM_DISCONNECTING */
if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
dev_err(&desc->intf->dev, "Error in flush path: %d\n",
desc->werr);

- return usb_translate_errors(desc->werr);
+ return test_bit(WDM_DISCONNECTING, &desc->flags) ?
+ -ENODEV :
+ usb_translate_errors(desc->werr);
}

static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
@@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
spin_lock_irqsave(&desc->iuspin, flags);
set_bit(WDM_DISCONNECTING, &desc->flags);
set_bit(WDM_READ, &desc->flags);
- /* to terminate pending flushes */
- clear_bit(WDM_IN_USE, &desc->flags);
spin_unlock_irqrestore(&desc->iuspin, flags);
wake_up_all(&desc->wait);
mutex_lock(&desc->rlock);

--
2.16.4

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
syzbot+d232cc...@syzkaller.appspotmail.com

Tested on:

commit: e06ce4da usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git

kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e

compiler: gcc (GCC) 9.0.0 20181231 (experimental)

patch: https://syzkaller.appspot.com/x/patch.diff?x=11893186600000

Note: testing is done by a robot and is best-effort only.

[Oliver Neukum]

Am Dienstag, den 20.08.2019, 12:44 +0200 schrieb Bjørn Mork :
> Oliver Neukum <one...@suse.com> writes:
>
> > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> > index 1656f5155ab8..a341081a5f47 100644
> > --- a/drivers/usb/class/cdc-wdm.c
> > +++ b/drivers/usb/class/cdc-wdm.c
> > @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
> > {
> > struct wdm_device *desc = file->private_data;
> >
> > - wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> > + wait_event(desc->wait,
> > + /*
> > + * needs both flags. We cannot do with one
> > + * because resetting it would cause a race
> > + * with write() yet we need to signal
> > + * a disconnect
> > + */
> > + !test_bit(WDM_IN_USE, &desc->flags) &&
> > + !test_bit(WDM_DISCONNECTING, &desc->flags));
>
>
> Makes sense. But isn't the WDM_DISCONNECTING test inverted?

You are right. I am making V3.

> > /* cannot dereference desc->intf if WDM_DISCONNECTING */
> > if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
> > dev_err(&desc->intf->dev, "Error in flush path: %d\n",
> > desc->werr);
> >
> > - return usb_translate_errors(desc->werr);
> > + return test_bit(WDM_DISCONNECTING, &desc->flags) ?
> > + -ENODEV :
> > + usb_translate_errors(desc->werr);
> > }
>
> Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
> here. How about
>
> if (test_bit(WDM_DISCONNECTING, &desc->flags))
> return -ENODEV;
> if (desc->werr < 0)
> dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);
> return usb_translate_errors(desc->werr);

Much better.

Regards
Oliver

[Oliver Neukum]

Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:

> syzbot has found a reproducer for the following crash on:

Hi Bjørn,

taking you into CC as you are affected.

V3: changes you suggested

#syz test: https://github.com/google/kasan.git e06ce4da

From a867eff99e3d4099c8e810c4ed2695f3ecdcab41 Mon Sep 17 00:00:00 2001

From: Oliver Neukum <one...@suse.com>
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse

In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.

The issue is as old as the driver.

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cc...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <one...@suse.com>
---
drivers/usb/class/cdc-wdm.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..f2cb5d401790 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,10 +588,20 @@ static int wdm_flush(struct file *file, fl_owner_t id)

{
struct wdm_device *desc = file->private_data;

- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+ wait_event(desc->wait,
+ /*
+ * needs both flags. We cannot do with one
+ * because resetting it would cause a race
+ * with write() yet we need to signal
+ * a disconnect
+ */
+ !test_bit(WDM_IN_USE, &desc->flags) &&

+ test_bit(WDM_DISCONNECTING, &desc->flags));

/* cannot dereference desc->intf if WDM_DISCONNECTING */

- if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
+ if (test_bit(WDM_DISCONNECTING, &desc->flags))
+ return -ENODEV;
+ if (desc->werr < 0)

dev_err(&desc->intf->dev, "Error in flush path: %d\n",
desc->werr);

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
syzbot+d232cc...@syzkaller.appspotmail.com

Tested on:

commit: e06ce4da usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git

kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e

compiler: gcc (GCC) 9.0.0 20181231 (experimental)

patch: https://syzkaller.appspot.com/x/patch.diff?x=15b207e2600000

[Bjørn Mork]

Oliver Neukum <one...@suse.com> writes:

> + wait_event(desc->wait,
> + /*
> + * needs both flags. We cannot do with one
> + * because resetting it would cause a race
> + * with write() yet we need to signal
> + * a disconnect
> + */
> + !test_bit(WDM_IN_USE, &desc->flags) &&
> + test_bit(WDM_DISCONNECTING, &desc->flags));

I'm confused now... Won't this condition always be false?
Should be

wait_event(desc->wait,
!test_bit(WDM_IN_USE, &desc->flags) ||
test_bit(WDM_DISCONNECTING, &desc->flags));


instead, or?



Bjørn

[Oliver Neukum]

<censored> <censored> ...

You are right.

Regards
Oliver

[Oliver Neukum]

Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:

> syzbot has found a reproducer for the following crash on:

Hi Bjørn,

taking you into CC as you are affected.

V4: <argh>

#syz test: https://github.com/google/kasan.git e06ce4da

From 8973b1216f931f4c7b82b02186caee9dcae16d24 Mon Sep 17 00:00:00 2001

From: Oliver Neukum <one...@suse.com>
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse

In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.

The issue is as old as the driver.

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cc...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <one...@suse.com>
---
drivers/usb/class/cdc-wdm.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c

index 1656f5155ab8..f9f7c8a5e091 100644

--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,10 +588,20 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;

- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));

+ wait_event(desc->wait,
+ /*
+ * needs both flags. We cannot do with one
+ * because resetting it would cause a race
+ * with write() yet we need to signal
+ * a disconnect
+ */
+ !test_bit(WDM_IN_USE, &desc->flags) ||

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
syzbot+d232cc...@syzkaller.appspotmail.com

Tested on:

commit: e06ce4da usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git

kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e

compiler: gcc (GCC) 9.0.0 20181231 (experimental)

patch: https://syzkaller.appspot.com/x/patch.diff?x=142198bc600000