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+53f029db71c19a473...@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
> /* 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: <sta...@vger.kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@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;
