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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> Reported-and-tested-by: syzbot+53f029db71c19a47325a@xxxxxxxxxxxxxxxxxxxxxxxxx
> CC: <stable@xxxxxxxxxxxxxxx>
> 
> ---
> 
> 
> [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@xxxxxxxxxxxxxxx>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx>

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;