[PATCH] USB: yurex: fix control-URB timeout handling

[PATCH] USB: yurex: fix control-URB timeout handling

[Johan Hovold]

Make sure to always cancel the control URB in write() so that it can be
reused after a timeout or spurious CMD_ACK.

Currently any further write requests after a timeout would fail after
triggering a WARN() in usb_submit_urb() when attempting to submit the
already active URB.

Reported-by: syzbot+e87ebe0f7913f71f2ea5@syzkaller.appspotmail.com
Fixes: 6bc235a2e24a ("USB: add driver for Meywa-Denki & Kayac YUREX")
Cc: stable <stable@vger.kernel.org>     # 2.6.37
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/yurex.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 73ebfa6e9715..c640f98d20c5 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -496,6 +496,9 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
 	finish_wait(&dev->waitq, &wait);
 
+	/* make sure URB is idle after timeout or (spurious) CMD_ACK */
+	usb_kill_urb(dev->cntl_urb);
+
 	mutex_unlock(&dev->io_mutex);
 
 	if (retval < 0) {
-- 
2.26.2

Re: [PATCH] USB: yurex: fix control-URB timeout handling

[Johan Hovold]

On Mon, Dec 14, 2020 at 11:44:44AM +0100, Johan Hovold wrote:
> Make sure to always cancel the control URB in write() so that it can be
> reused after a timeout or spurious CMD_ACK.
> 
> Currently any further write requests after a timeout would fail after
> triggering a WARN() in usb_submit_urb() when attempting to submit the
> already active URB.
> 
> Reported-by: syzbot+e87ebe0f7913f71f2ea5@syzkaller.appspotmail.com
> Fixes: 6bc235a2e24a ("USB: add driver for Meywa-Denki & Kayac YUREX")
> Cc: stable <stable@vger.kernel.org>     # 2.6.37
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Forgot linux-usb...

Let's try this too:

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing

>  drivers/usb/misc/yurex.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
> index 73ebfa6e9715..c640f98d20c5 100644
> --- a/drivers/usb/misc/yurex.c
> +++ b/drivers/usb/misc/yurex.c
> @@ -496,6 +496,9 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
>  		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
>  	finish_wait(&dev->waitq, &wait);
>  
> +	/* make sure URB is idle after timeout or (spurious) CMD_ACK */
> +	usb_kill_urb(dev->cntl_urb);
> +
>  	mutex_unlock(&dev->io_mutex);
>  
>  	if (retval < 0) {

Re: WARNING in yurex_write/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in yurex_write/usb_submit_urb

------------[ cut here ]------------
URB 00000000d1c13d63 submitted while active
WARNING: CPU: 1 PID: 12383 at drivers/usb/core/urb.c:378 usb_submit_urb+0x1228/0x14e0 drivers/usb/core/urb.c:378
Modules linked in:
CPU: 1 PID: 12383 Comm: syz-executor.2 Not tainted 5.10.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:usb_submit_urb+0x1228/0x14e0 drivers/usb/core/urb.c:378
Code: 89 de e8 1b cd 3b fc 84 db 0f 85 da f4 ff ff e8 fe d4 3b fc 4c 89 fe 48 c7 c7 20 59 e1 89 c6 05 14 8c a4 07 01 e8 a4 b3 78 03 <0f> 0b e9 b8 f4 ff ff c7 44 24 14 01 00 00 00 e9 6f f5 ff ff 41 bd
RSP: 0018:ffffc90001c9fcb8 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff888020788000 RSI: ffffffff8158c835 RDI: fffff52000393f89
RBP: 1ffff92000393fa9 R08: 0000000000000001 R09: ffff8880b9f30627
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880276ea400
R13: 00000000fffffff0 R14: ffff8880276ea4e8 R15: ffff888012dca100
FS:  00007fad9e6f7700(0000) GS:ffff8880b9f00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005587bf0f4160 CR3: 00000000252a1000 CR4: 00000000001506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 yurex_write+0x3f4/0x840 drivers/usb/misc/yurex.c:494
 vfs_write+0x28e/0xa30 fs/read_write.c:603
 ksys_write+0x12d/0x250 fs/read_write.c:658
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45e159
Code: 0d b4 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 db b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fad9e6f6c68 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 000000000045e159
RDX: 0000000000000001 RSI: 0000000020000740 RDI: 0000000000000004
RBP: 000000000119bfc0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000119bf8c
R13: 00007ffc9fc101ef R14: 00007fad9e6f79c0 R15: 000000000119bf8c


Tested on:

commit:         a256e240 usb: phy: convert comma to semicolon
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
console output: https://syzkaller.appspot.com/x/log.txt?x=17357137500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4536e7f93c2bc8e9
dashboard link: https://syzkaller.appspot.com/bug?extid=e87ebe0f7913f71f2ea5
compiler:       gcc (GCC) 10.1.0-syz 20200507

Re: WARNING in yurex_write/usb_submit_urb

[Johan Hovold]

On Mon, Dec 14, 2020 at 06:48:03AM -0800, syzbot wrote:
> Hello,
> 
> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> WARNING in yurex_write/usb_submit_urb

It appears syzbot never tested the patch from the thread. Probably using
it's mail interface incorrectly, I don't know and I don't have time to
investigate. The patch itself is correct.

Johan

Re: WARNING in yurex_write/usb_submit_urb

[Andrey Konovalov]

On Mon, Dec 14, 2020 at 4:02 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Dec 14, 2020 at 06:48:03AM -0800, syzbot wrote:
> > Hello,
> >
> > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > WARNING in yurex_write/usb_submit_urb
>
> It appears syzbot never tested the patch from the thread. Probably using
> it's mail interface incorrectly, I don't know and I don't have time to
> investigate. The patch itself is correct.

Hi Johan,

I wasn't CCed on the testing request, so I can't say what exactly was wrong.

Could you send me the patch you were trying to test?

Thanks!

Re: WARNING in yurex_write/usb_submit_urb

[Johan Hovold]

On Mon, Dec 14, 2020 at 04:06:49PM +0100, Andrey Konovalov wrote:
> On Mon, Dec 14, 2020 at 4:02 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Mon, Dec 14, 2020 at 06:48:03AM -0800, syzbot wrote:
> > > Hello,
> > >
> > > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > > WARNING in yurex_write/usb_submit_urb
> >
> > It appears syzbot never tested the patch from the thread. Probably using
> > it's mail interface incorrectly, I don't know and I don't have time to
> > investigate. The patch itself is correct.
> 
> Hi Johan,
> 
> I wasn't CCed on the testing request, so I can't say what exactly was wrong.

Here's the patch and the "syz test" command in a reply:

	https://lore.kernel.org/r/20201214104444.28386-1-johan@kernel.org

Probably needs to go in the same mail, right?

How about including the command needed to test a patch in the syzbot
report mail to assist the casual user of its interfaces? I had to browse
the web page you link to and still got it wrong apparently.

> Could you send me the patch you were trying to test?

Does this work better:

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing

Johan


From 3bb77b2ac604d70b06f45a850e326dda9c99c9cd Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 14 Dec 2020 11:30:53 +0100
Subject: [PATCH] USB: yurex: fix control-URB timeout handling

Make sure to always cancel the control URB in write() so that it can be
reused after a timeout or spurious CMD_ACK.

Currently any further write requests after a timeout would fail after
triggering a WARN() in usb_submit_urb() when attempting to submit the
already active URB.

Reported-by: syzbot+e87ebe0f7913f71f2ea5@syzkaller.appspotmail.com
Fixes: 6bc235a2e24a ("USB: add driver for Meywa-Denki & Kayac YUREX")
Cc: stable <stable@vger.kernel.org>     # 2.6.37
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/misc/yurex.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 73ebfa6e9715..c640f98d20c5 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -496,6 +496,9 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
 	finish_wait(&dev->waitq, &wait);
 
+	/* make sure URB is idle after timeout or (spurious) CMD_ACK */
+	usb_kill_urb(dev->cntl_urb);
+
 	mutex_unlock(&dev->io_mutex);
 
 	if (retval < 0) {
-- 
2.26.2

Re: WARNING in yurex_write/usb_submit_urb

[syzbot]

Hello,

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

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

Tested on:

commit:         a256e240 usb: phy: convert comma to semicolon
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
kernel config:  https://syzkaller.appspot.com/x/.config?x=4536e7f93c2bc8e9
dashboard link: https://syzkaller.appspot.com/bug?extid=e87ebe0f7913f71f2ea5
compiler:       gcc (GCC) 10.1.0-syz 20200507
patch:          https://syzkaller.appspot.com/x/patch.diff?x=11a9f703500000

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

Re: WARNING in yurex_write/usb_submit_urb

[Andrey Konovalov]

On Mon, Dec 14, 2020 at 4:16 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Dec 14, 2020 at 04:06:49PM +0100, Andrey Konovalov wrote:
> > On Mon, Dec 14, 2020 at 4:02 PM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > On Mon, Dec 14, 2020 at 06:48:03AM -0800, syzbot wrote:
> > > > Hello,
> > > >
> > > > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > > > WARNING in yurex_write/usb_submit_urb
> > >
> > > It appears syzbot never tested the patch from the thread. Probably using
> > > it's mail interface incorrectly, I don't know and I don't have time to
> > > investigate. The patch itself is correct.
> >
> > Hi Johan,
> >
> > I wasn't CCed on the testing request, so I can't say what exactly was wrong.
>
> Here's the patch and the "syz test" command in a reply:
>
>         https://lore.kernel.org/r/20201214104444.28386-1-johan@kernel.org
>
> Probably needs to go in the same mail, right?

Yes, both the syz test command and the patch must be in the same
email, which is sent as a response to the initial report.

> How about including the command needed to test a patch in the syzbot
> report mail to assist the casual user of its interfaces? I had to browse
> the web page you link to and still got it wrong apparently.

I think it's deliberately not included to not overload the report
email with too many details.

> > Could you send me the patch you were trying to test?
>
> Does this work better:
>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing

This worked :)

Thanks!