LinuxTV Patchwork general protection fault in smsusb_init_device

login
register
mail settings
Submitter Alan Stern
Date May 6, 2019, 8:41 p.m.
Message ID <Pine.LNX.4.44L0.1905061638380.1585-100000@iolanthe.rowland.org>
Download mbox | patch
Permalink /patch/56067/
State RFC
Headers show

Comments

Alan Stern - May 6, 2019, 8:41 p.m.
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(-)
syzbot - May 6, 2019, 9:21 p.m.
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.
Johan Hovold - May 7, 2019, 8:34 a.m.
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
Alan Stern - May 7, 2019, 2:42 p.m.
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
Johan Hovold - May 7, 2019, 3:07 p.m.
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

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;

Privacy Policy