general protection fault in smsusb_init_device

general protection fault in smsusb_init_device

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    d34f9519 usb-fuzzer: main usb gadget fuzzer driver
git tree:       https://github.com/google/kasan/tree/usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000
kernel config:  https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20
dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000

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

usb 1-1: config 0 descriptor??
usb 1-1: string descriptor 0 read error: -71
smsusb:smsusb_probe: board id=18, interface number 0
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN PTI
CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
RIP: 0010:smsusb_init_device+0x366/0x937  
drivers/media/usb/siano/smsusb.c:429
Code: 48 c1 ea 03 80 3c 02 00 74 05 e8 24 1e 66 f7 4d 8b b6 f0 04 00 00 b8  
ff ff 37 00 48 c1 e0 2a 49 8d 7e 04 48 89 fa 48 c1 ea 03 <8a> 14 02 48 89  
f8 83 e0 07 ff c0 38 d0 7c 09 84 d2 74 05 e8 b1 1d
RSP: 0018:ffff8880a86570d0 EFLAGS: 00010247
RAX: dffffc0000000000 RBX: ffff88809a81b300 RCX: ffffffff8a42b5b3
RDX: 0000000000000000 RSI: ffffffff8a42b6a3 RDI: 0000000000000004
RBP: ffff88808ca70000 R08: ffff8880a8503100 R09: ffff8880a8657130
R10: ffffed10150cae34 R11: ffff8880a86571a7 R12: ffff88809a81be54
R13: ffff88809a81be5c R14: 0000000000000000 R15: ffff88808ca70000
FS:  0000000000000000(0000) GS:ffff8880ad100000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f1ad259d000 CR3: 000000009a3aa000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  smsusb_probe+0xd64/0xe08 drivers/media/usb/siano/smsusb.c:570
  usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361
  really_probe+0x2da/0xb10 drivers/base/dd.c:509
  driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
  __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
  bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
  __device_attach+0x223/0x3a0 drivers/base/dd.c:844
  bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
  device_add+0xad2/0x16e0 drivers/base/core.c:2106
  usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021
  generic_probe+0xa2/0xda drivers/usb/core/generic.c:210
  usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266
  really_probe+0x2da/0xb10 drivers/base/dd.c:509
  driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
  __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
  bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
  __device_attach+0x223/0x3a0 drivers/base/dd.c:844
  bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
  device_add+0xad2/0x16e0 drivers/base/core.c:2106
  usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534
  hub_port_connect drivers/usb/core/hub.c:5089 [inline]
  hub_port_connect_change drivers/usb/core/hub.c:5204 [inline]
  port_event drivers/usb/core/hub.c:5350 [inline]
  hub_event+0x1398/0x3b00 drivers/usb/core/hub.c:5432
  process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
  worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
  kthread+0x313/0x420 kernel/kthread.c:253
  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
Modules linked in:
---[ end trace 6175778b99b10882 ]---
RIP: 0010:smsusb_init_device+0x366/0x937  
drivers/media/usb/siano/smsusb.c:429
Code: 48 c1 ea 03 80 3c 02 00 74 05 e8 24 1e 66 f7 4d 8b b6 f0 04 00 00 b8  
ff ff 37 00 48 c1 e0 2a 49 8d 7e 04 48 89 fa 48 c1 ea 03 <8a> 14 02 48 89  
f8 83 e0 07 ff c0 38 d0 7c 09 84 d2 74 05 e8 b1 1d
RSP: 0018:ffff8880a86570d0 EFLAGS: 00010247
RAX: dffffc0000000000 RBX: ffff88809a81b300 RCX: ffffffff8a42b5b3
RDX: 0000000000000000 RSI: ffffffff8a42b6a3 RDI: 0000000000000004
RBP: ffff88808ca70000 R08: ffff8880a8503100 R09: ffff8880a8657130
R10: ffffed10150cae34 R11: ffff8880a86571a7 R12: ffff88809a81be54
R13: ffff88809a81be5c R14: 0000000000000000 R15: ffff88808ca70000
FS:  0000000000000000(0000) GS:ffff8880ad100000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f1ad259d000 CR3: 000000009a3aa000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
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: general protection fault in smsusb_init_device

[Alan Stern]

On Thu, 18 Apr 2019, syzbot wrote:

> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    d34f9519 usb-fuzzer: main usb gadget fuzzer driver
> git tree:       https://github.com/google/kasan/tree/usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20
> dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> 
> usb 1-1: config 0 descriptor??
> usb 1-1: string descriptor 0 read error: -71
> smsusb:smsusb_probe: board id=18, interface number 0
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN PTI
> CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
> Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:smsusb_init_device+0x366/0x937  
> drivers/media/usb/siano/smsusb.c:429
> Code: 48 c1 ea 03 80 3c 02 00 74 05 e8 24 1e 66 f7 4d 8b b6 f0 04 00 00 b8  
> ff ff 37 00 48 c1 e0 2a 49 8d 7e 04 48 89 fa 48 c1 ea 03 <8a> 14 02 48 89  
> f8 83 e0 07 ff c0 38 d0 7c 09 84 d2 74 05 e8 b1 1d
> RSP: 0018:ffff8880a86570d0 EFLAGS: 00010247
> RAX: dffffc0000000000 RBX: ffff88809a81b300 RCX: ffffffff8a42b5b3
> RDX: 0000000000000000 RSI: ffffffff8a42b6a3 RDI: 0000000000000004
> RBP: ffff88808ca70000 R08: ffff8880a8503100 R09: ffff8880a8657130
> R10: ffffed10150cae34 R11: ffff8880a86571a7 R12: ffff88809a81be54
> R13: ffff88809a81be5c R14: 0000000000000000 R15: ffff88808ca70000
> FS:  0000000000000000(0000) GS:ffff8880ad100000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f1ad259d000 CR3: 000000009a3aa000 CR4: 00000000001406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>   smsusb_probe+0xd64/0xe08 drivers/media/usb/siano/smsusb.c:570

The reason for this bug is clear.  The code in smsusb_probe() at line
429 does this:

		dev->response_alignment =
		    le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) -
		    sizeof(struct sms_msg_hdr);

which assumes there really is an ep1-IN endpoint.  If there isn't, the 
code crashes.

Testing that the endpoint exists is easy enough, but I'm not sure how 
this test should be integrated with the rest of the function.  Someone 
who knows the code better ought to be able to do it with no trouble.

Alan Stern

Re: general protection fault in smsusb_init_device

[Alan Stern]

On Thu, 18 Apr 2019, syzbot wrote:

> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    d34f9519 usb-fuzzer: main usb gadget fuzzer driver
> git tree:       https://github.com/google/kasan/tree/usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20
> dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> 
> usb 1-1: config 0 descriptor??
> usb 1-1: string descriptor 0 read error: -71
> smsusb:smsusb_probe: board id=18, interface number 0
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN PTI
> CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
> Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:smsusb_init_device+0x366/0x937  
> drivers/media/usb/siano/smsusb.c:429

The driver assumes endpoint 1in exists, and doesn't check the existence 
of the endpoints it uses.

Alan Stern


#syz test: https://github.com/google/kasan.git usb-fuzzer

 drivers/media/usb/siano/smsusb.c |   32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Index: usb-devel/drivers/media/usb/siano/smsusb.c
===================================================================
--- usb-devel.orig/drivers/media/usb/siano/smsusb.c
+++ usb-devel/drivers/media/usb/siano/smsusb.c
@@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb
 	struct smsusb_device_t *dev;
 	void *mdev;
 	int i, rc;
+	int in_maxp;
 
 	/* create device object */
 	dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
@@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb
 	dev->udev = interface_to_usbdev(intf);
 	dev->state = SMSUSB_DISCONNECTED;
 
+	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
+		struct usb_endpoint_descriptor *desc =
+				&intf->cur_altsetting->endpoint[i].desc;
+
+		if (desc->bEndpointAddress & USB_DIR_IN) {
+			dev->in_ep = desc->bEndpointAddress;
+			in_maxp = usb_endpoint_maxp(desc);
+		} else {
+			dev->out_ep = desc->bEndpointAddress;
+		}
+	}
+
+	pr_debug("in_ep = %02x, out_ep = %02x\n",
+		dev->in_ep, dev->out_ep);
+	if (!dev->in_ep || !dev->out_ep)	/* Missing endpoints? */
+		return -EINVAL;
+
 	params.device_type = sms_get_board(board_id)->type;
 
 	switch (params.device_type) {
@@ -425,24 +443,12 @@ static int smsusb_init_device(struct usb
 		/* fall-thru */
 	default:
 		dev->buffer_size = USB2_BUFFER_SIZE;
-		dev->response_alignment =
-		    le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) -
-		    sizeof(struct sms_msg_hdr);
+		dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
 
 		params.flags |= SMS_DEVICE_FAMILY2;
 		break;
 	}
 
-	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
-		if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN)
-			dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
-		else
-			dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
-	}
-
-	pr_debug("in_ep = %02x, out_ep = %02x\n",
-		dev->in_ep, dev->out_ep);
-
 	params.device = &dev->udev->dev;
 	params.usb_device = dev->udev;
 	params.buffer_size = dev->buffer_size;

Re: general protection fault in smsusb_init_device

[syzbot]

Hello,

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

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

Tested on:

commit:         43151d6c usb-fuzzer: main usb gadget fuzzer driver
git tree:       https://github.com/google/kasan.git usb-fuzzer
kernel config:  https://syzkaller.appspot.com/x/.config?x=274aad0cf966c3bc
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=12c3fda4a00000

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

Re: general protection fault in smsusb_init_device

[Johan Hovold]

On Mon, May 06, 2019 at 04:41:41PM -0400, Alan Stern wrote:
> On Thu, 18 Apr 2019, syzbot wrote:
> 
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    d34f9519 usb-fuzzer: main usb gadget fuzzer driver
> > git tree:       https://github.com/google/kasan/tree/usb-fuzzer
> > console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20
> > dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> > 
> > usb 1-1: config 0 descriptor??
> > usb 1-1: string descriptor 0 read error: -71
> > smsusb:smsusb_probe: board id=18, interface number 0
> > kasan: CONFIG_KASAN_INLINE enabled
> > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > general protection fault: 0000 [#1] SMP KASAN PTI
> > CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
> > Google 01/01/2011
> > Workqueue: usb_hub_wq hub_event
> > RIP: 0010:smsusb_init_device+0x366/0x937  
> > drivers/media/usb/siano/smsusb.c:429
> 
> The driver assumes endpoint 1in exists, and doesn't check the existence 
> of the endpoints it uses.
> 
> Alan Stern
> 
> 
> #syz test: https://github.com/google/kasan.git usb-fuzzer
> 
>  drivers/media/usb/siano/smsusb.c |   32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> Index: usb-devel/drivers/media/usb/siano/smsusb.c
> ===================================================================
> --- usb-devel.orig/drivers/media/usb/siano/smsusb.c
> +++ usb-devel/drivers/media/usb/siano/smsusb.c
> @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb
>  	struct smsusb_device_t *dev;
>  	void *mdev;
>  	int i, rc;
> +	int in_maxp;
>  
>  	/* create device object */
>  	dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
> @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb
>  	dev->udev = interface_to_usbdev(intf);
>  	dev->state = SMSUSB_DISCONNECTED;
>  
> +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> +		struct usb_endpoint_descriptor *desc =
> +				&intf->cur_altsetting->endpoint[i].desc;
> +
> +		if (desc->bEndpointAddress & USB_DIR_IN) {
> +			dev->in_ep = desc->bEndpointAddress;
> +			in_maxp = usb_endpoint_maxp(desc);
> +		} else {
> +			dev->out_ep = desc->bEndpointAddress;
> +		}
> +	}
> +
> +	pr_debug("in_ep = %02x, out_ep = %02x\n",
> +		dev->in_ep, dev->out_ep);
> +	if (!dev->in_ep || !dev->out_ep)	/* Missing endpoints? */
> +		return -EINVAL;

Looks like you're now leaking dev here, and so is the current code in
the later error paths.

Since this return value will be returned from probe, you may want to use
-ENXIO or -ENODEV instead of -EINVAL.

Looks good otherwise.

Johan

Re: general protection fault in smsusb_init_device

[Alan Stern]

On Tue, 7 May 2019, Johan Hovold wrote:

> On Mon, May 06, 2019 at 04:41:41PM -0400, Alan Stern wrote:
> > On Thu, 18 Apr 2019, syzbot wrote:
> > 
> > > Hello,
> > > 
> > > syzbot found the following crash on:
> > > 
> > > HEAD commit:    d34f9519 usb-fuzzer: main usb gadget fuzzer driver
> > > git tree:       https://github.com/google/kasan/tree/usb-fuzzer
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a
> > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000
> > > 
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> > > 
> > > usb 1-1: config 0 descriptor??
> > > usb 1-1: string descriptor 0 read error: -71
> > > smsusb:smsusb_probe: board id=18, interface number 0
> > > kasan: CONFIG_KASAN_INLINE enabled
> > > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > > general protection fault: 0000 [#1] SMP KASAN PTI
> > > CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
> > > Google 01/01/2011
> > > Workqueue: usb_hub_wq hub_event
> > > RIP: 0010:smsusb_init_device+0x366/0x937  
> > > drivers/media/usb/siano/smsusb.c:429
> > 
> > The driver assumes endpoint 1in exists, and doesn't check the existence 
> > of the endpoints it uses.
> > 
> > Alan Stern
> > 
> > 
> > #syz test: https://github.com/google/kasan.git usb-fuzzer
> > 
> >  drivers/media/usb/siano/smsusb.c |   32 +++++++++++++++++++-------------
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > Index: usb-devel/drivers/media/usb/siano/smsusb.c
> > ===================================================================
> > --- usb-devel.orig/drivers/media/usb/siano/smsusb.c
> > +++ usb-devel/drivers/media/usb/siano/smsusb.c
> > @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb
> >  	struct smsusb_device_t *dev;
> >  	void *mdev;
> >  	int i, rc;
> > +	int in_maxp;
> >  
> >  	/* create device object */
> >  	dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
> > @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb
> >  	dev->udev = interface_to_usbdev(intf);
> >  	dev->state = SMSUSB_DISCONNECTED;
> >  
> > +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> > +		struct usb_endpoint_descriptor *desc =
> > +				&intf->cur_altsetting->endpoint[i].desc;
> > +
> > +		if (desc->bEndpointAddress & USB_DIR_IN) {
> > +			dev->in_ep = desc->bEndpointAddress;
> > +			in_maxp = usb_endpoint_maxp(desc);
> > +		} else {
> > +			dev->out_ep = desc->bEndpointAddress;
> > +		}
> > +	}
> > +
> > +	pr_debug("in_ep = %02x, out_ep = %02x\n",
> > +		dev->in_ep, dev->out_ep);
> > +	if (!dev->in_ep || !dev->out_ep)	/* Missing endpoints? */
> > +		return -EINVAL;
> 
> Looks like you're now leaking dev here, and so is the current code in
> the later error paths.
> 
> Since this return value will be returned from probe, you may want to use
> -ENXIO or -ENODEV instead of -EINVAL.
> 
> Looks good otherwise.

Thanks for the review.  You're right about the memory leak (although 
you're wrong about the later error paths: smsusb_term_device() 
deallocates dev).  And -ENODEV does seem like a better return code.

I'll update the patch as you suggest.

Alan Stern

Re: general protection fault in smsusb_init_device

[Johan Hovold]

On Tue, May 07, 2019 at 10:42:58AM -0400, Alan Stern wrote:
> On Tue, 7 May 2019, Johan Hovold wrote:

> > > @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb
> > >  	dev->udev = interface_to_usbdev(intf);
> > >  	dev->state = SMSUSB_DISCONNECTED;
> > >  
> > > +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> > > +		struct usb_endpoint_descriptor *desc =
> > > +				&intf->cur_altsetting->endpoint[i].desc;
> > > +
> > > +		if (desc->bEndpointAddress & USB_DIR_IN) {
> > > +			dev->in_ep = desc->bEndpointAddress;
> > > +			in_maxp = usb_endpoint_maxp(desc);
> > > +		} else {
> > > +			dev->out_ep = desc->bEndpointAddress;
> > > +		}
> > > +	}
> > > +
> > > +	pr_debug("in_ep = %02x, out_ep = %02x\n",
> > > +		dev->in_ep, dev->out_ep);
> > > +	if (!dev->in_ep || !dev->out_ep)	/* Missing endpoints? */
> > > +		return -EINVAL;
> > 
> > Looks like you're now leaking dev here, and so is the current code in
> > the later error paths.
> > 
> > Since this return value will be returned from probe, you may want to use
> > -ENXIO or -ENODEV instead of -EINVAL.
> > 
> > Looks good otherwise.
> 
> Thanks for the review.  You're right about the memory leak (although 
> you're wrong about the later error paths: smsusb_term_device() 
> deallocates dev).

Indeed, I missed the free in smsusb_term_device(). Sorry about that.

Johan

[PATCH] media: usb: siano: Fix general protection fault in smsusb

[Alan Stern]

The syzkaller USB fuzzer found a general-protection-fault bug in the
smsusb part of the Siano DVB driver.  The fault occurs during probe
because the driver assumes without checking that the device has both
IN and OUT endpoints and the IN endpoint is ep1.

By slightly rearranging the driver's initialization code, we can make
the appropriate checks early on and thus avoid the problem.  If the
expected endpoints aren't present, the new code safely returns -ENODEV
from the probe routine.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
CC: <stable@vger.kernel.org>

---


[as1897]


 drivers/media/usb/siano/smsusb.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

Index: usb-devel/drivers/media/usb/siano/smsusb.c
===================================================================
--- usb-devel.orig/drivers/media/usb/siano/smsusb.c
+++ usb-devel/drivers/media/usb/siano/smsusb.c
@@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb
 	struct smsusb_device_t *dev;
 	void *mdev;
 	int i, rc;
+	int in_maxp;
 
 	/* create device object */
 	dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
@@ -411,6 +412,24 @@ static int smsusb_init_device(struct usb
 	dev->udev = interface_to_usbdev(intf);
 	dev->state = SMSUSB_DISCONNECTED;
 
+	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
+		struct usb_endpoint_descriptor *desc =
+				&intf->cur_altsetting->endpoint[i].desc;
+
+		if (desc->bEndpointAddress & USB_DIR_IN) {
+			dev->in_ep = desc->bEndpointAddress;
+			in_maxp = usb_endpoint_maxp(desc);
+		} else {
+			dev->out_ep = desc->bEndpointAddress;
+		}
+	}
+
+	pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep);
+	if (!dev->in_ep || !dev->out_ep) {	/* Missing endpoints? */
+		smsusb_term_device(intf);
+		return -ENODEV;
+	}
+
 	params.device_type = sms_get_board(board_id)->type;
 
 	switch (params.device_type) {
@@ -425,24 +444,12 @@ static int smsusb_init_device(struct usb
 		/* fall-thru */
 	default:
 		dev->buffer_size = USB2_BUFFER_SIZE;
-		dev->response_alignment =
-		    le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) -
-		    sizeof(struct sms_msg_hdr);
+		dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
 
 		params.flags |= SMS_DEVICE_FAMILY2;
 		break;
 	}
 
-	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
-		if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN)
-			dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
-		else
-			dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
-	}
-
-	pr_debug("in_ep = %02x, out_ep = %02x\n",
-		dev->in_ep, dev->out_ep);
-
 	params.device = &dev->udev->dev;
 	params.usb_device = dev->udev;
 	params.buffer_size = dev->buffer_size;

Re: [PATCH] media: usb: siano: Fix general protection fault in smsusb

[Johan Hovold]

On Tue, May 07, 2019 at 12:39:47PM -0400, Alan Stern wrote:
> The syzkaller USB fuzzer found a general-protection-fault bug in the
> smsusb part of the Siano DVB driver.  The fault occurs during probe
> because the driver assumes without checking that the device has both
> IN and OUT endpoints and the IN endpoint is ep1.
> 
> By slightly rearranging the driver's initialization code, we can make
> the appropriate checks early on and thus avoid the problem.  If the
> expected endpoints aren't present, the new code safely returns -ENODEV
> from the probe routine.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> CC: <stable@vger.kernel.org>

Reviewed-by: Johan Hovold <johan@kernel.org>

Re: [PATCH] media: usb: siano: Fix general protection fault in smsusb

[Mauro Carvalho Chehab]

Em Tue, 7 May 2019 12:39:47 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> escreveu:

> The syzkaller USB fuzzer found a general-protection-fault bug in the
> smsusb part of the Siano DVB driver.  The fault occurs during probe
> because the driver assumes without checking that the device has both
> IN and OUT endpoints and the IN endpoint is ep1.
> 
> By slightly rearranging the driver's initialization code, we can make
> the appropriate checks early on and thus avoid the problem.  If the
> expected endpoints aren't present, the new code safely returns -ENODEV
> from the probe routine.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> CC: <stable@vger.kernel.org>
> 
> ---
> 
> 
> [as1897]
> 
> 
>  drivers/media/usb/siano/smsusb.c |   33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> Index: usb-devel/drivers/media/usb/siano/smsusb.c
> ===================================================================
> --- usb-devel.orig/drivers/media/usb/siano/smsusb.c
> +++ usb-devel/drivers/media/usb/siano/smsusb.c
> @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb
>  	struct smsusb_device_t *dev;
>  	void *mdev;
>  	int i, rc;
> +	int in_maxp;
>  
>  	/* create device object */
>  	dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
> @@ -411,6 +412,24 @@ static int smsusb_init_device(struct usb
>  	dev->udev = interface_to_usbdev(intf);
>  	dev->state = SMSUSB_DISCONNECTED;
>  
> +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> +		struct usb_endpoint_descriptor *desc =
> +				&intf->cur_altsetting->endpoint[i].desc;
> +
> +		if (desc->bEndpointAddress & USB_DIR_IN) {
> +			dev->in_ep = desc->bEndpointAddress;
> +			in_maxp = usb_endpoint_maxp(desc);
> +		} else {
> +			dev->out_ep = desc->bEndpointAddress;
> +		}
> +	}
> +
> +	pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep);
> +	if (!dev->in_ep || !dev->out_ep) {	/* Missing endpoints? */
> +		smsusb_term_device(intf);
> +		return -ENODEV;
> +	}
> +
>  	params.device_type = sms_get_board(board_id)->type;
>  
>  	switch (params.device_type) {
> @@ -425,24 +444,12 @@ static int smsusb_init_device(struct usb
>  		/* fall-thru */
>  	default:
>  		dev->buffer_size = USB2_BUFFER_SIZE;
> -		dev->response_alignment =
> -		    le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) -
> -		    sizeof(struct sms_msg_hdr);
> +		dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
>  
>  		params.flags |= SMS_DEVICE_FAMILY2;
>  		break;
>  	}
>  
> -	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> -		if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN)
> -			dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
> -		else
> -			dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
> -	}
> -
> -	pr_debug("in_ep = %02x, out_ep = %02x\n",
> -		dev->in_ep, dev->out_ep);
> -
>  	params.device = &dev->udev->dev;
>  	params.usb_device = dev->udev;
>  	params.buffer_size = dev->buffer_size;
> 

Patch looks correct, and I'm applying it. It exposes another potential
problem though: what happens if sizeof(desc.wMaxPacketSize) < sizeof(struct sms_msg_hdr)?

I'm enclosing a followup patch that should solve this situation
(and clean up a sparse warning).

Thanks,
Mauro

[PATCH] media: smsusb: better handle optional alignment

Most Siano devices require an alignment for the response.

Changeset f3be52b0056a ("media: usb: siano: Fix general protection fault in smsusb")
changed the logic with gets such aligment, but it now produces a
sparce warning:

drivers/media/usb/siano/smsusb.c: In function 'smsusb_init_device':
drivers/media/usb/siano/smsusb.c:447:37: warning: 'in_maxp' may be used uninitialized in this function [-Wmaybe-uninitialized]
  447 |   dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
      |                             ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~

The sparse message itself is bogus, but a broken (or fake) USB
eeprom could produce a negative value for response_alignment.

So, change the code in order to check if the result is not
negative.

Fixes: f3be52b0056a ("media: usb: siano: Fix general protection fault in smsusb")
CC: <stable@vger.kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 27ad14a3f831..e39f3f40dfdd 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -400,7 +400,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 	struct smsusb_device_t *dev;
 	void *mdev;
 	int i, rc;
-	int in_maxp;
+	int align = 0;
 
 	/* create device object */
 	dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
@@ -418,14 +418,14 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 
 		if (desc->bEndpointAddress & USB_DIR_IN) {
 			dev->in_ep = desc->bEndpointAddress;
-			in_maxp = usb_endpoint_maxp(desc);
+			align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr);
 		} else {
 			dev->out_ep = desc->bEndpointAddress;
 		}
 	}
 
 	pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep);
-	if (!dev->in_ep || !dev->out_ep) {	/* Missing endpoints? */
+	if (!dev->in_ep || !dev->out_ep || align < 0) {  /* Missing endpoints? */
 		smsusb_term_device(intf);
 		return -ENODEV;
 	}
@@ -444,7 +444,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 		/* fall-thru */
 	default:
 		dev->buffer_size = USB2_BUFFER_SIZE;
-		dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
+		dev->response_alignment = align;
 
 		params.flags |= SMS_DEVICE_FAMILY2;
 		break;

Re: [PATCH] media: usb: siano: Fix general protection fault in smsusb

[Alan Stern]

On Fri, 24 May 2019, Mauro Carvalho Chehab wrote:

> Em Tue, 7 May 2019 12:39:47 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> escreveu:
> 
> > The syzkaller USB fuzzer found a general-protection-fault bug in the
> > smsusb part of the Siano DVB driver.  The fault occurs during probe
> > because the driver assumes without checking that the device has both
> > IN and OUT endpoints and the IN endpoint is ep1.
> > 
> > By slightly rearranging the driver's initialization code, we can make
> > the appropriate checks early on and thus avoid the problem.  If the
> > expected endpoints aren't present, the new code safely returns -ENODEV
> > from the probe routine.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com
> > CC: <stable@vger.kernel.org>

> Patch looks correct, and I'm applying it. It exposes another potential
> problem though: what happens if sizeof(desc.wMaxPacketSize) < sizeof(struct sms_msg_hdr)?
> 
> I'm enclosing a followup patch that should solve this situation
> (and clean up a sparse warning).
> 
> Thanks,
> Mauro

Your points are well taken.

However, Greg KH has already taken the original patch and a fix for the 
sparse warning into his tree.  I guess the two of you should figure out 
how best to straighten this out.

Alan Stern