| Message ID | Pine.LNX.4.44L0.1905071237310.1632-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 31e0456de5be379b10fea0fa94a681057114a96e |
| Headers | show |
| Series | media: usb: siano: Fix general protection fault in smsusb | expand |
On Tue, May 07, 2019 at 12:39:47PM -0400, Alan Stern wrote: > 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@rowland.harvard.edu> > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > CC: <stable@vger.kernel.org> Reviewed-by: Johan Hovold <johan@kernel.org>
Em Tue, 7 May 2019 12:39:47 -0400 (EDT) Alan Stern <stern@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 <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > CC: <stable@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: <stable@vger.kernel.org> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@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;
On Fri, 24 May 2019, Mauro Carvalho Chehab wrote: > Em Tue, 7 May 2019 12:39:47 -0400 (EDT) > Alan Stern <stern@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 <stern@rowland.harvard.edu> > > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > > CC: <stable@vger.kernel.org> > 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 Your points are well taken. However, Greg KH has already taken the original patch and a fix for the sparse warning into his tree. I guess the two of you should figure out how best to straighten this out. Alan Stern
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;
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@rowland.harvard.edu> Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com CC: <stable@vger.kernel.org> --- [as1897] drivers/media/usb/siano/smsusb.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)