| Message ID | 1445946694-2931-1-git-send-email-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, Oliver, all.
Unfortunately, there are at least 4 different concerns about this patch:
1) "!dev->actconfig->interface[ifnum]" won't catch a case where the value is not
NULL but some garbage. This way the system may crash with "general protection fault"
later. I've hit this case during my tests.
2) "(ifnum >= USB_MAXINTERFACES)" does not cover all the error conditions. "ifnum"
should be compared to "dev->actconfig->desc.bNumInterfaces". I.e. compared to the
number of "struct usb_interface" kzalloc()-ed, not to USB_MAXINTERFACES.
3) If returned like this ("return -ENODEV;") there is a "usb_device" leak. There is
usb_get_dev() in the first line of usbvision_probe() but no usb_put_dev() on this
failure path. Commit "afd270d1" addresses exactly this case in other failure paths.
4) There is a bug of the same type several lines below with number of endpoints. The
code is accessing hard-coded second endpoint ("interface->endpoint[1].desc") which
may not exist. It would be great to handle this in the same patch too.
I've just posted my version of the patch for the same issue to the list (subject:
"[media] usbvision: fix crash on detecting device with invalid configuration",
Message-Id: <1447696511-17704-2-git-send-email-vdronov@redhat.com>). I believe it
resolves all the above concerns.
Best regards,
Vladis Dronov | Red Hat, Inc.| Product Security Engineer
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
An HTTP URL for the alternate patch mentioned: http://www.spinics.net/lists/linux-media/msg94831.html Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c index b693206..ad33d99 100644 --- a/drivers/media/usb/usbvision/usbvision-video.c +++ b/drivers/media/usb/usbvision/usbvision-video.c @@ -1461,6 +1461,13 @@ static int usbvision_probe(struct usb_interface *intf, printk(KERN_INFO "%s: %s found\n", __func__, usbvision_device_data[model].model_string); + /* + * this is a security check. + * an exploit using an incorrect bInterfaceNumber is known + */ + if (ifnum >= USB_MAXINTERFACES || !dev->actconfig->interface[ifnum]) + return -ENODEV; + if (usbvision_device_data[model].interface >= 0) interface = &dev->actconfig->interface[usbvision_device_data[model].interface]->altsetting[0]; else
This fixes the crash reported in: http://seclists.org/bugtraq/2015/Oct/35 The interface number needs a sanity check. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/media/usb/usbvision/usbvision-video.c | 7 +++++++ 1 file changed, 7 insertions(+)