lkml.org 
[lkml]   [2019]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: general protection fault in smsusb_init_device
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

\
 
 \ /
  Last update: 2019-05-07 16:45    [W:0.092 / U:0.480 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site