Hello,
syzbot found the following issue on:
HEAD commit: f87b564686ee dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
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=119f3aaf480000
kernel config: https://syzkaller.appspot.com/x/.config?x=6d59dd45f9349215
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17db7007480000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1670f2b3480000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/162f005fbb8d/disk-f87b5646.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/868c38dbb85a/vmlinux-f87b5646.xz
kernel image: https://storage.googleapis.com/syzbot-assets/e560670dfb35/bzImage-f87b5646.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
usb 1-1: Product: syz
usb 1-1: Manufacturer: syz
usb 1-1: SerialNumber: syz
usb 1-1: config 0 descriptor??
------------[ cut here ]------------
URB ffff888112baaf00 submitted while active
WARNING: CPU: 0 PID: 12 at drivers/usb/core/urb.c:379 usb_submit_urb+0x14ec/0x1880 drivers/usb/core/urb.c:379
Modules linked in:
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 6.2.0-rc7-syzkaller-00232-gf87b564686ee #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/21/2023
Workqueue: events request_firmware_work_func
RIP: 0010:usb_submit_urb+0x14ec/0x1880 drivers/usb/core/urb.c:379
Code: 89 de e8 87 86 88 fd 84 db 0f 85 a3 f3 ff ff e8 0a 8a 88 fd 4c 89 fe 48 c7 c7 00 2d a8 86 c6 05 14 8a 14 05 01 e8 18 06 19 02 <0f> 0b e9 81 f3 ff ff 48 89 7c 24 40 e8 e3 89 88 fd 48 8b 7c 24 40
RSP: 0018:ffffc900000cfa00 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff8881002dd400 RSI: ffffffff812db84c RDI: fffff52000019f32
RBP: ffff888112baaf00 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: 00000000c0028200
R13: 0000000000000010 R14: 00000000fffffff0 R15: ffff888112baaf00
FS: 0000000000000000(0000) GS:ffff8881f6800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f165ac57130 CR3: 000000011215a000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
usb_tx_block+0x241/0x2e0 drivers/net/wireless/marvell/libertas/if_usb.c:436
if_usb_issue_boot_command drivers/net/wireless/marvell/libertas/if_usb.c:766 [inline]
if_usb_prog_firmware+0x531/0xe30 drivers/net/wireless/marvell/libertas/if_usb.c:859
lbs_fw_loaded drivers/net/wireless/marvell/libertas/firmware.c:23 [inline]
helper_firmware_cb drivers/net/wireless/marvell/libertas/firmware.c:80 [inline]
helper_firmware_cb+0x1e9/0x2c0 drivers/net/wireless/marvell/libertas/firmware.c:64
request_firmware_work_func+0x130/0x240 drivers/base/firmware_loader/main.c:1107
process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289
worker_thread+0x669/0x1090 kernel/workqueue.c:2436
kthread+0x2ee/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
</TASK>
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
Tested on:
commit: f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
console output: https://syzkaller.appspot.com/x/log.txt?x=13c8d577480000
kernel config: https://syzkaller.appspot.com/x/.config?x=6d59dd45f9349215
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=113a83d0c80000
Note: testing is done by a robot and is best-effort only.
On mercoled? 15 febbraio 2023 12:05:15 CET Hillf Danton wrote:
> On Tue, 14 Feb 2023 23:00:47 -0800
>
> > syzbot found the following issue on:
> >
> > HEAD commit: f87b564686ee dt-bindings: usb: amlogic,meson-g12a-usb-
ctrl..
> > git tree:
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1670f2b3480000
> Kill urb in flight after submitting it.
>
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> f87b564686ee
>
> --- x/drivers/net/wireless/marvell/libertas/if_usb.c
> +++ y/drivers/net/wireless/marvell/libertas/if_usb.c
> @@ -763,9 +763,7 @@ static int if_usb_issue_boot_command(str
> memset(bootcmd->pad, 0, sizeof(bootcmd->pad));
>
> /* Issue command */
> - usb_tx_block(cardp, cardp->ep_out_buf, sizeof(*bootcmd));
> -
> - return 0;
> + return usb_tx_block(cardp, cardp->ep_out_buf, sizeof(*bootcmd));
> }
>
>
> @@ -853,10 +851,12 @@ restart:
> }
>
> cardp->bootcmdresp = 0;
> + ret = if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
> + if (ret)
> + goto done;
I think that you are changing the logic here (please read below)...
> do {
> int j = 0;
> i++;
> - if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
Don't we need to call if_usb_issue_boot_command() in a loop in order to retry
the command?
> /* wait for command response */
> do {
> j++;
> @@ -864,6 +864,8 @@ restart:
> } while (cardp->bootcmdresp == 0 && j < 10);
> } while (cardp->bootcmdresp == 0 && i < 5);
>
> + usb_kill_urb(cardp->tx_urb);
> +
I'm not an expert in the USB core, anyway calling usb_kill_urb() looks good to
me, but I think we should call it after each call of
if_usb_issue_boot_command() in the above outer loop.
> if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
> /* Return to normal operation */
> ret = -EOPNOTSUPP;
> --
Can the following work?
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
f87b564686ee
diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/
wireless/marvell/libertas/if_usb.c
index 20436a289d5c..626357d0c7b0 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -859,6 +859,7 @@ static void if_usb_prog_firmware(struct lbs_private *priv,
int ret,
j++;
msleep_interruptible(100);
} while (cardp->bootcmdresp == 0 && j < 10);
+ usb_kill_urb(cardp->tx_urb):
} while (cardp->bootcmdresp == 0 && i < 5);
if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
--
Thanks,
Fabio
Hello,
syzbot tried to test the proposed patch but the build/boot failed:
drivers/net/wireless/marvell/libertas/if_usb.c:865:43: error: expected ';' before ':' token
Tested on:
commit: f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=1414acf0c80000
On gioved? 16 febbraio 2023 08:44:18 CET syzbot wrote:
> Hello,
>
> syzbot tried to test the proposed patch but the build/boot failed:
>
> drivers/net/wireless/marvell/libertas/if_usb.c:865:43: error: expected ';'
> before ':' token
>
>
> Tested on:
>
> commit: f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
> git tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/
usb.git
> dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils
> for Debian) 2.35.2 patch:
> https://syzkaller.appspot.com/x/patch.diff?x=1414acf0c80000
Sorry for this syntax error :-(
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
f87b564686ee
diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/
wireless/marvell/libertas/if_usb.c
index 20436a289d5c..e03a5dcf6dab 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -859,6 +859,7 @@ static void if_usb_prog_firmware(struct lbs_private *priv,
int ret,
j++;
msleep_interruptible(100);
} while (cardp->bootcmdresp == 0 && j < 10);
+ usb_kill_urb(cardp->tx_urb);
} while (cardp->bootcmdresp == 0 && i < 5);
if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
--
Fabio
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
Tested on:
commit: f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
console output: https://syzkaller.appspot.com/x/log.txt?x=160ab7d0c80000
kernel config: https://syzkaller.appspot.com/x/.config?x=6d59dd45f9349215
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=16fe1f10c80000
Note: testing is done by a robot and is best-effort only.
On gioved? 16 febbraio 2023 09:18:34 CET Hillf Danton wrote:
> Fabio!
>
> On Thu, 16 Feb 2023 07:54:08 +0100 Fabio M. De Francesco
> <[email protected]>
> > > do {
> > >
> > > int j =3D 0;
> > > i++;
> > >
> > > - if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
> >
> > Don't we need to call if_usb_issue_boot_command() in a loop in order to
> > retry
> > the command?
>
> Nope certainly because of no sense made by sending it again, given no
> response this round.
>
Your argument looks reasonable but...
For what regards subsystems/drivers whose I'm not expert I always assume that
the authors know what they do despite bugs. I mean that looks more probable
that they have reasons to issue several calls to if_usb_issue_boot_command() /
usb_submit_urb in a loop. May be that those usb_submit_urb get lost in some
particular conditions, since they decide to try if_usb_issue_boot_command() in
a loop (but forget to kill the URB before next iteration).
I have no reasons to think you are wrong. However I don't understand the
reason that made you leave the loops untouched (except the line with the call
to if_usb_issue_boot_command().
I suppose that, if you confirm that we have no reasons to reiterate that call,
you should also leave only one loop waiting for response.
Am I missing something?
Thanks,
Fabio