Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* WARNING in wdm_write/usb_submit_urb
@ 2019-08-19 14:48 syzbot
  2019-08-20  0:50 ` syzbot
  0 siblings, 1 reply; 14+ messages in thread
From: syzbot @ 2019-08-19 14:48 UTC (permalink / raw)
  To: andreyknvl, gregkh, gustavo, linux-kernel, linux-usb, syzkaller-bugs

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+d232cca6ec42c2edb3fc@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 syzkaller@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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: WARNING in wdm_write/usb_submit_urb
  2019-08-19 14:48 WARNING in wdm_write/usb_submit_urb syzbot
@ 2019-08-20  0:50 ` syzbot
  2019-08-20 10:31   ` Oliver Neukum
                     ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: syzbot @ 2019-08-20  0:50 UTC (permalink / raw)
  To: andreyknvl, gregkh, gustavo, linux-kernel, linux-usb, syzkaller-bugs

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+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
Kernel Offset: disabled
Rebooting in 86400 seconds..


^ 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: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	[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: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	[flat|nested] 14+ messages in thread

* Re: WARNING in wdm_write/usb_submit_urb
  2019-08-20 10:59   ` Oliver Neukum
@ 2019-08-20 11:18     ` syzbot
  0 siblings, 0 replies; 14+ messages in thread
From: syzbot @ 2019-08-20 11:18 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=11893186600000

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 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  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	[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	[flat|nested] 14+ messages in thread

* Re: WARNING in wdm_write/usb_submit_urb
  2019-08-20 13:41   ` Oliver Neukum
@ 2019-08-20 14:00     ` syzbot
  0 siblings, 0 replies; 14+ messages in thread
From: syzbot @ 2019-08-20 14:00 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=142198bc600000

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 14:48 WARNING in wdm_write/usb_submit_urb syzbot
2019-08-20  0:50 ` syzbot
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
2019-08-20 10:59   ` Oliver Neukum
2019-08-20 11:18     ` syzbot
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
2019-08-20 13:41   ` Oliver Neukum
2019-08-20 14:00     ` syzbot

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox