WARNING in dln2_start_rx_urbs/usb_submit_urb

WARNING in dln2_start_rx_urbs/usb_submit_urb

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    b1aa9d83 usb: raw: add raw-gadget interface
git tree:       https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=12599e8ae00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=79de80330003b5f7
dashboard link: https://syzkaller.appspot.com/bug?extid=48a2851be24583b864dc
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=166cd064e00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=144c7adce00000

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

usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
usb 1-1: config 0 descriptor??
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 1 PID: 83 at drivers/usb/core/urb.c:477  
usb_submit_urb+0x1188/0x13b0 drivers/usb/core/urb.c:477
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 83 Comm: kworker/1:2 Not tainted 5.4.0-rc6+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0xca/0x13e lib/dump_stack.c:113
  panic+0x2aa/0x6e1 kernel/panic.c:221
  __warn.cold+0x2f/0x33 kernel/panic.c:582
  report_bug+0x27b/0x2f0 lib/bug.c:195
  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+0x1188/0x13b0 drivers/usb/core/urb.c:477
Code: 4d 85 ed 74 2c e8 b8 21 ef fd 4c 89 f7 e8 40 34 1f ff 41 89 d8 44 89  
e1 4c 89 ea 48 89 c6 48 c7 c7 40 77 14 86 e8 cd bc c4 fd <0f> 0b e9 20 f4  
ff ff e8 8c 21 ef fd 4c 89 f2 48 b8 00 00 00 00 00
RSP: 0018:ffff8881d89ef180 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff8128c9cd RDI: ffffed103b13de22
RBP: ffff8881d53ae450 R08: ffff8881d8c5c800 R09: fffffbfff11ab3ae
R10: fffffbfff11ab3ad R11: ffffffff88d59d6f R12: 0000000000000003
R13: ffff8881d3571318 R14: ffff8881d037c0a0 R15: ffff8881d53ae200
  dln2_start_rx_urbs+0x6


---
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

Re: WARNING in dln2_start_rx_urbs/usb_submit_urb

[Greg KH]

On Mon, Dec 09, 2019 at 12:26:04PM +0000, Lee Jones wrote:
> On Wed, 20 Nov 2019, Oliver Neukum wrote:
> 
> > #syz test: https://github.com/google/kasan.git b1aa9d83
> > 
> > From 6f3de1e2c53fcee54d90a6891df9cea763eca86b Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum <oneukum@suse.com>
> > Date: Wed, 20 Nov 2019 16:15:13 +0100
> > Subject: [PATCH] mfd: dln2: more sanity checking for endpoints
> > 
> > It is not enough to check for the number of endpoints.
> > The types must also be correct.
> > 
> > Reported-by: syzbot+48a2851be24583b864dc@syzkaller.appspotmail.com
> > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > ---
> >  drivers/mfd/dln2.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> Looks okay in principle.
> 
> It would be great if one of the USB guys could take a quick look.
> 
> Is this not something that could be done at a subsystem level?

We have functions in the USB core for this, look at
usb_find_common_endpoints() for an example of this.  You can convert the
driver to use this, or use the usb helper functions that Oliver is using
here in a much simpler patch :)

thanks,

greg k-h

Re: WARNING in dln2_start_rx_urbs/usb_submit_urb

[Lee Jones]

On Wed, 20 Nov 2019, Oliver Neukum wrote:

> #syz test: https://github.com/google/kasan.git b1aa9d83
> 
> From 6f3de1e2c53fcee54d90a6891df9cea763eca86b Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Wed, 20 Nov 2019 16:15:13 +0100
> Subject: [PATCH] mfd: dln2: more sanity checking for endpoints
> 
> It is not enough to check for the number of endpoints.
> The types must also be correct.
> 
> Reported-by: syzbot+48a2851be24583b864dc@syzkaller.appspotmail.com
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/mfd/dln2.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

Looks okay in principle.

It would be great if one of the USB guys could take a quick look.

Is this not something that could be done at a subsystem level?

> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 381593fbe50f..7841c11411d0 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -722,6 +722,8 @@ static int dln2_probe(struct usb_interface *interface,
>  		      const struct usb_device_id *usb_id)
>  {
>  	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct usb_endpoint_descriptor *epin;
> +	struct usb_endpoint_descriptor *epout;
>  	struct device *dev = &interface->dev;
>  	struct dln2_dev *dln2;
>  	int ret;
> @@ -731,12 +733,19 @@ static int dln2_probe(struct usb_interface *interface,
>  	    hostif->desc.bNumEndpoints < 2)
>  		return -ENODEV;
>  
> +	epin = &hostif->endpoint[0].desc;
> +	epout = &hostif->endpoint[1].desc;
> +	if (!usb_endpoint_is_bulk_out(epout))
> +		return -ENODEV;
> +	if (!usb_endpoint_is_bulk_in(epin))
> +		return -ENODEV;
> +
>  	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
>  	if (!dln2)
>  		return -ENOMEM;
>  
> -	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
> -	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
> +	dln2->ep_out = epout->bEndpointAddress;
> +	dln2->ep_in = epin->bEndpointAddress;
>  	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
>  	dln2->interface = interface;
>  	usb_set_intfdata(interface, dln2);

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: WARNING in dln2_start_rx_urbs/usb_submit_urb

[syzbot]

Hello,

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

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

Tested on:

commit:         b1aa9d83 usb: raw: add raw-gadget interface
git tree:       https://github.com/google/kasan.git
kernel config:  https://syzkaller.appspot.com/x/.config?x=79de80330003b5f7
dashboard link: https://syzkaller.appspot.com/bug?extid=48a2851be24583b864dc
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1555d686e00000

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

Re: WARNING in dln2_start_rx_urbs/usb_submit_urb

[Oliver Neukum]

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

From 6f3de1e2c53fcee54d90a6891df9cea763eca86b Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 20 Nov 2019 16:15:13 +0100
Subject: [PATCH] mfd: dln2: more sanity checking for endpoints

It is not enough to check for the number of endpoints.
The types must also be correct.

Reported-by: syzbot+48a2851be24583b864dc@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/mfd/dln2.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 381593fbe50f..7841c11411d0 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -722,6 +722,8 @@ static int dln2_probe(struct usb_interface *interface,
 		      const struct usb_device_id *usb_id)
 {
 	struct usb_host_interface *hostif = interface->cur_altsetting;
+	struct usb_endpoint_descriptor *epin;
+	struct usb_endpoint_descriptor *epout;
 	struct device *dev = &interface->dev;
 	struct dln2_dev *dln2;
 	int ret;
@@ -731,12 +733,19 @@ static int dln2_probe(struct usb_interface *interface,
 	    hostif->desc.bNumEndpoints < 2)
 		return -ENODEV;
 
+	epin = &hostif->endpoint[0].desc;
+	epout = &hostif->endpoint[1].desc;
+	if (!usb_endpoint_is_bulk_out(epout))
+		return -ENODEV;
+	if (!usb_endpoint_is_bulk_in(epin))
+		return -ENODEV;
+
 	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
 	if (!dln2)
 		return -ENOMEM;
 
-	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
-	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
+	dln2->ep_out = epout->bEndpointAddress;
+	dln2->ep_in = epin->bEndpointAddress;
 	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
 	dln2->interface = interface;
 	usb_set_intfdata(interface, dln2);
-- 
2.16.4