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+53f029...@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 syzk...@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

[Alan Stern]

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

[Alan Stern]

On Thu, 18 Apr 2019, syzbot wrote:

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;

[syzbot]

Hello,

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

Reported-and-tested-by:
syzbot+53f029...@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.

[Johan Hovold]

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

[Alan Stern]

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

[Johan Hovold]

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

Johan

[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 <st...@rowland.harvard.edu>
Reported-and-tested-by: syzbot+53f029...@syzkaller.appspotmail.com
CC: <sta...@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

[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 <st...@rowland.harvard.edu>
> Reported-and-tested-by: syzbot+53f029...@syzkaller.appspotmail.com
> CC: <sta...@vger.kernel.org>

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

[Mauro Carvalho Chehab]

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

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: <sta...@vger.kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab...@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;

[Alan Stern]

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

> Em Tue, 7 May 2019 12:39:47 -0400 (EDT)
> Alan Stern <st...@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 <st...@rowland.harvard.edu>
> > Reported-and-tested-by: syzbot+53f029...@syzkaller.appspotmail.com
> > CC: <sta...@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