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