* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 0:50 ` syzbot
@ 2019-08-20 10:31 ` Oliver Neukum
2019-08-20 10:44 ` Bjørn Mork
2019-08-20 10:45 ` syzbot
2019-08-20 10:59 ` Oliver Neukum
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: Oliver Neukum @ 2019-08-20 10:31 UTC (permalink / raw)
To: syzbot, gustavo, andreyknvl, syzkaller-bugs, gregkh, linux-usb
Cc: Bjørn Mork
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.
> 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+d232cca6ec42c2edb3fc@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
#syz test: https://github.com/google/kasan.git e06ce4da
From 38dda97aa4820a60c680374edbab0323cfcb0193 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@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+d232cca6ec42c2edb3fc@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 10:31 ` Oliver Neukum
@ 2019-08-20 10:44 ` Bjørn Mork
2019-08-20 12:27 ` Oliver Neukum
2019-08-20 10:45 ` syzbot
1 sibling, 1 reply; 14+ messages in thread
From: Bjørn Mork @ 2019-08-20 10:44 UTC (permalink / raw)
To: Oliver Neukum
Cc: syzbot, gustavo, andreyknvl, syzkaller-bugs, gregkh, linux-usb
Oliver Neukum <oneukum@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 10:44 ` Bjørn Mork
@ 2019-08-20 12:27 ` Oliver Neukum
0 siblings, 0 replies; 14+ messages in thread
From: Oliver Neukum @ 2019-08-20 12:27 UTC (permalink / raw)
To: Bjørn Mork
Cc: gustavo, andreyknvl, syzkaller-bugs, gregkh, syzbot, linux-usb
Am Dienstag, den 20.08.2019, 12:44 +0200 schrieb Bjørn Mork :
> Oliver Neukum <oneukum@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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 10:31 ` Oliver Neukum
2019-08-20 10:44 ` Bjørn Mork
@ 2019-08-20 10:45 ` syzbot
1 sibling, 0 replies; 14+ messages in thread
From: syzbot @ 2019-08-20 10:45 UTC (permalink / raw)
To: andreyknvl, bjorn, gregkh, gustavo, linux-usb, oneukum, syzkaller-bugs
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 0:50 ` syzbot
2019-08-20 10:31 ` Oliver Neukum
@ 2019-08-20 10:59 ` Oliver Neukum
2019-08-20 11:18 ` syzbot
2019-08-20 12:28 ` Oliver Neukum
2019-08-20 13:41 ` Oliver Neukum
3 siblings, 1 reply; 14+ messages in thread
From: Oliver Neukum @ 2019-08-20 10:59 UTC (permalink / raw)
To: syzbot, gustavo, andreyknvl, syzkaller-bugs, gregkh, linux-usb
Cc: Bjørn Mork
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
> 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+d232cca6ec42c2edb3fc@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
#syz test: https://github.com/google/kasan.git e06ce4da
From 8d0c7a38b1a4883e7bdab76b06a9e564fc466506 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@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+d232cca6ec42c2edb3fc@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 0:50 ` syzbot
2019-08-20 10:31 ` Oliver Neukum
2019-08-20 10:59 ` Oliver Neukum
@ 2019-08-20 12:28 ` Oliver Neukum
2019-08-20 12:48 ` syzbot
2019-08-20 13:13 ` Bjørn Mork
2019-08-20 13:41 ` Oliver Neukum
3 siblings, 2 replies; 14+ messages in thread
From: Oliver Neukum @ 2019-08-20 12:28 UTC (permalink / raw)
To: syzbot, gustavo, andreyknvl, syzkaller-bugs, gregkh, linux-usb
Cc: Bjørn Mork
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
> 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+d232cca6ec42c2edb3fc@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
#syz test: https://github.com/google/kasan.git e06ce4da
From a867eff99e3d4099c8e810c4ed2695f3ecdcab41 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@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+d232cca6ec42c2edb3fc@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@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);
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 12:28 ` Oliver Neukum
@ 2019-08-20 12:48 ` syzbot
2019-08-20 13:13 ` Bjørn Mork
1 sibling, 0 replies; 14+ messages in thread
From: syzbot @ 2019-08-20 12:48 UTC (permalink / raw)
To: andreyknvl, bjorn, gregkh, gustavo, linux-usb, oneukum, syzkaller-bugs
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
syzbot+d232cca6ec42c2edb3fc@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
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 12:28 ` Oliver Neukum
2019-08-20 12:48 ` syzbot
@ 2019-08-20 13:13 ` Bjørn Mork
2019-08-20 13:40 ` Oliver Neukum
1 sibling, 1 reply; 14+ messages in thread
From: Bjørn Mork @ 2019-08-20 13:13 UTC (permalink / raw)
To: Oliver Neukum
Cc: syzbot, gustavo, andreyknvl, syzkaller-bugs, gregkh, linux-usb
Oliver Neukum <oneukum@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 13:13 ` Bjørn Mork
@ 2019-08-20 13:40 ` Oliver Neukum
0 siblings, 0 replies; 14+ messages in thread
From: Oliver Neukum @ 2019-08-20 13:40 UTC (permalink / raw)
To: Bjørn Mork
Cc: gustavo, andreyknvl, syzkaller-bugs, gregkh, syzbot, linux-usb
Am Dienstag, den 20.08.2019, 15:13 +0200 schrieb Bjørn Mork :
> Oliver Neukum <oneukum@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?
<censored> <censored> ...
You are right.
Regards
Oliver
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: WARNING in wdm_write/usb_submit_urb
2019-08-20 0:50 ` syzbot
` (2 preceding siblings ...)
2019-08-20 12:28 ` Oliver Neukum
@ 2019-08-20 13:41 ` Oliver Neukum
2019-08-20 14:00 ` syzbot
3 siblings, 1 reply; 14+ messages in thread
From: Oliver Neukum @ 2019-08-20 13:41 UTC (permalink / raw)
To: syzbot, gustavo, andreyknvl, syzkaller-bugs, gregkh, linux-usb
Cc: Bjørn Mork
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>
> 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+d232cca6ec42c2edb3fc@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
#syz test: https://github.com/google/kasan.git e06ce4da
From 8973b1216f931f4c7b82b02186caee9dcae16d24 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@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+d232cca6ec42c2edb3fc@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@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) ||
+ 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);
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread