On Fri, 2016-08-26 at 18:02 +0000, Binyamin Sharet (bsharet) wrote:
> I think the reason is that in case of quirks == NO_UNION_NORMAL it gets the data
> and control interfaces and then jumps to skip_normal probe, in which case there are no
> checks whether data_interface, data_interface->cur_altsetting or control_interface are NULL.
>
> Also, even in the case where quirks != NO_UNION_NORMAL, if the control and the data
> interfaces are not the same, there is no check whether data_interface->cur_altsetting is NULL.
> (before the first line of code in skip_normal_probe) which may cause the first check to dereference
> a NULL pointer.
Hi,
could you test this updated patch?
Regards
Oliver
From 4274cf2a7f8bf0b97b6d5ecc27882b9d946bf7f0 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
Date: Wed, 17 Aug 2016 15:19:28 +0200
Subject: [PATCH] cdc-acm: added sanity checking for probe()
This is analternative to eccf2a4e6b64d249929acc1f7aaa2ab0fb199d3d
which inadvertedly fixes an oops in probe by malformed descriptors.
The patch is too extensive to backport to stable.
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
drivers/usb/class/cdc-acm.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index ba6b978..d54e2c7 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1002,6 +1002,8 @@ static int acm_probe(struct usb_interface *intf,
}
if (!buflen) {
+ if (!intf->cur_altsetting || !intf->cur_altsetting->endpoint)
+ return -EINVAL;
if (intf->cur_altsetting->endpoint &&
intf->cur_altsetting->endpoint->extralen &&
intf->cur_altsetting->endpoint->extra) {
@@ -1069,6 +1071,8 @@ next_desc:
data_interface = usb_ifnum_to_if(usb_dev, (data_interface_num = call_interface_num));
control_interface = intf;
} else {
+ if (!intf->cur_altsetting)
+ return -ENODEV;
if (intf->cur_altsetting->desc.bNumEndpoints != 3) {
dev_dbg(&intf->dev,"No union descriptor, giving up\n");
return -ENODEV;
@@ -1098,15 +1102,22 @@ next_desc:
combined_interfaces = 1;
/* a popular other OS doesn't use it */
quirks |= NO_CAP_LINE;
+ if (!data_interface->cur_altsetting)
+ return -EINVAL;
if (data_interface->cur_altsetting->desc.bNumEndpoints != 3) {
dev_err(&intf->dev, "This needs exactly 3 endpoints\n");
return -EINVAL;
}
look_for_collapsed_interface:
+ if (!data_interface->cur_altsetting)
+ return -EINVAL;
for (i = 0; i < 3; i++) {
struct usb_endpoint_descriptor *ep;
ep = &data_interface->cur_altsetting->endpoint[i].desc;
+ if (!ep)
+ return -ENODEV;
+
if (usb_endpoint_is_int_in(ep))
epctrl = ep;
else if (usb_endpoint_is_bulk_out(ep))
@@ -1125,8 +1136,12 @@ look_for_collapsed_interface:
skip_normal_probe:
/*workaround for switched interfaces */
+ if (!data_interface->cur_altsetting)
+ return -EINVAL;
if (data_interface->cur_altsetting->desc.bInterfaceClass
!= CDC_DATA_INTERFACE_TYPE) {
+ if (!control_interface->cur_altsetting)
+ return -EINVAL;
if (control_interface->cur_altsetting->desc.bInterfaceClass
== CDC_DATA_INTERFACE_TYPE) {
struct usb_interface *t;
@@ -1152,6 +1167,7 @@ skip_normal_probe:
if (data_interface->cur_altsetting->desc.bNumEndpoints < 2 ||
+ !control_interface->cur_altsetting ||
control_interface->cur_altsetting->desc.bNumEndpoints == 0)
return -EINVAL;
@@ -1159,6 +1175,8 @@ skip_normal_probe:
epread = &data_interface->cur_altsetting->endpoint[0].desc;
epwrite = &data_interface->cur_altsetting->endpoint[1].desc;
+ if (!epctrl || !epread || !epwrite)
+ return -ENODEV;
/* workaround for switched endpoints */
if (!usb_endpoint_dir_in(epread)) {
--
2.1.4
