[Linuxwacom-devel] [PATCH input-wacom] Prevent NULL derefence when bNumEndpoints is 0

Discussion:

Jason Gerecke

2016-05-03 22:33:43 UTC

If a device with a malformed USB interface descriptor that indicates
zero enpoints are available is probed by our driver, the system will
crash due to NULL dereferences. This patch implements the fix suggested
in the mentioned Red Hat bugs.

Ref: CVE-2016-3139
Fixes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1556883
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1283375
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1283377

Signed-off-by: Jason Gerecke <***@wacom.com>
---
Open question: RHEL has no plans to fix this in their tree -- do we want
to fix it in input-wacom?

2.6.30/wacom_sys.c | 6 ++++++
2.6.36/wacom_sys.c | 6 ++++++
2.6.38/wacom_sys.c | 6 ++++++
3.7/wacom_sys.c | 6 ++++++
4 files changed, 24 insertions(+)

diff --git a/2.6.30/wacom_sys.c b/2.6.30/wacom_sys.c
index c12cb02..473292c 100644
--- a/2.6.30/wacom_sys.c
+++ b/2.6.30/wacom_sys.c
@@ -798,6 +798,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i

wacom_wac->input = input_dev;

+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;

/* Retrieve the physical and logical size for OEM devices */
diff --git a/2.6.36/wacom_sys.c b/2.6.36/wacom_sys.c
index eda6175..1d90a91 100644
--- a/2.6.36/wacom_sys.c
+++ b/2.6.36/wacom_sys.c
@@ -885,6 +885,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i

wacom_wac->input = input_dev;

+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;

/* Retrieve the physical and logical size for touch devices */
diff --git a/2.6.38/wacom_sys.c b/2.6.38/wacom_sys.c
index 7902e21..190e5bc 100644
--- a/2.6.38/wacom_sys.c
+++ b/2.6.38/wacom_sys.c
@@ -1664,6 +1664,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
usb_make_path(dev, wacom->phys, sizeof(wacom->phys));
strlcat(wacom->phys, "/input0", sizeof(wacom->phys));

+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;

/* set the default size in case we do not get them from hid */
diff --git a/3.7/wacom_sys.c b/3.7/wacom_sys.c
index 3047773..da086ed 100644
--- a/3.7/wacom_sys.c
+++ b/3.7/wacom_sys.c
@@ -1662,6 +1662,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
usb_make_path(dev, wacom->phys, sizeof(wacom->phys));
strlcat(wacom->phys, "/input0", sizeof(wacom->phys));

+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;

/* set the default size in case we do not get them from hid */

--
2.8.2

Peter Hutterer

2016-05-04 09:44:33 UTC

Permalink

Post by Jason Gerecke
If a device with a malformed USB interface descriptor that indicates
zero enpoints are available is probed by our driver, the system will

typo, endpoints

Post by Jason Gerecke
crash due to NULL dereferences. This patch implements the fix suggested
in the mentioned Red Hat bugs.
Ref: CVE-2016-3139
Fixes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1556883
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1283375
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1283377
---
Open question: RHEL has no plans to fix this in their tree -- do we want
to fix it in input-wacom?

yes. RHEL has its own schedule, don't take it as a reference unless you
*specifically* work on something you intend to get into RHEL only. There are
a variety of other ways how this patch could end up in a future RHEL
version.

Cheers,
Peter

Post by Jason Gerecke
2.6.30/wacom_sys.c | 6 ++++++
2.6.36/wacom_sys.c | 6 ++++++
2.6.38/wacom_sys.c | 6 ++++++
3.7/wacom_sys.c | 6 ++++++
4 files changed, 24 insertions(+)
diff --git a/2.6.30/wacom_sys.c b/2.6.30/wacom_sys.c
index c12cb02..473292c 100644
--- a/2.6.30/wacom_sys.c
+++ b/2.6.30/wacom_sys.c
@@ -798,6 +798,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
wacom_wac->input = input_dev;
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* Retrieve the physical and logical size for OEM devices */
diff --git a/2.6.36/wacom_sys.c b/2.6.36/wacom_sys.c
index eda6175..1d90a91 100644
--- a/2.6.36/wacom_sys.c
+++ b/2.6.36/wacom_sys.c
@@ -885,6 +885,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
wacom_wac->input = input_dev;
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* Retrieve the physical and logical size for touch devices */
diff --git a/2.6.38/wacom_sys.c b/2.6.38/wacom_sys.c
index 7902e21..190e5bc 100644
--- a/2.6.38/wacom_sys.c
+++ b/2.6.38/wacom_sys.c
@@ -1664,6 +1664,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
usb_make_path(dev, wacom->phys, sizeof(wacom->phys));
strlcat(wacom->phys, "/input0", sizeof(wacom->phys));
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* set the default size in case we do not get them from hid */
diff --git a/3.7/wacom_sys.c b/3.7/wacom_sys.c
index 3047773..da086ed 100644
--- a/3.7/wacom_sys.c
+++ b/3.7/wacom_sys.c
@@ -1662,6 +1662,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
usb_make_path(dev, wacom->phys, sizeof(wacom->phys));
strlcat(wacom->phys, "/input0", sizeof(wacom->phys));
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* set the default size in case we do not get them from hid */
--
2.8.2
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Linuxwacom-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

Ping Cheng

2016-05-04 16:29:27 UTC

Permalink

Post by Peter Hutterer

Post by Jason Gerecke
If a device with a malformed USB interface descriptor that indicates
zero enpoints are available is probed by our driver, the system will

typo, endpoints

It's a bug fix. We need the patch at least for input-wacom.

Since the patch was initially created by Vladis Dronov
(https://bugzilla.redhat.com/attachment.cgi?id=1099056&action=diff),
shouldn't we get Vladis' s-o-b?

Post by Peter Hutterer
yes. RHEL has its own schedule, don't take it as a reference unless you
*specifically* work on something you intend to get into RHEL only. There are
a variety of other ways how this patch could end up in a future RHEL
version.

This patch only affects kernels older than 3.17. If RHEL only
backports patches from kernel.org, the fix won't be in kernel 3.10
unless we submit a patch for those longterm stable versions.

Post by Peter Hutterer

I think we want to validate bNumEndpoints before creating other
objects. Something like below would get the job done:

diff --git a/3.7/wacom_sys.c b/3.7/wacom_sys.c
index 3047773..8afeeb2 100644
--- a/3.7/wacom_sys.c
+++ b/3.7/wacom_sys.c
@@ -1630,6 +1630,10 @@ static int wacom_probe(struct usb_interface
*intf, const struct usb_device_id *i
if (!id->driver_info)
return -EINVAL;

+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1)
+ return -EINVAL;
+
wacom = kzalloc(sizeof(struct wacom), GFP_KERNEL);
if (!wacom)
return -ENOMEM;

Either way, you get my

Reviewed-by: Ping Cheng <***@wacom.com>

Cheers,

Ping

Post by Peter Hutterer

Post by Jason Gerecke
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* Retrieve the physical and logical size for OEM devices */
diff --git a/2.6.36/wacom_sys.c b/2.6.36/wacom_sys.c
index eda6175..1d90a91 100644
--- a/2.6.36/wacom_sys.c
+++ b/2.6.36/wacom_sys.c
@@ -885,6 +885,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
wacom_wac->input = input_dev;
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* Retrieve the physical and logical size for touch devices */
diff --git a/2.6.38/wacom_sys.c b/2.6.38/wacom_sys.c
index 7902e21..190e5bc 100644
--- a/2.6.38/wacom_sys.c
+++ b/2.6.38/wacom_sys.c
@@ -1664,6 +1664,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
usb_make_path(dev, wacom->phys, sizeof(wacom->phys));
strlcat(wacom->phys, "/input0", sizeof(wacom->phys));
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* set the default size in case we do not get them from hid */
diff --git a/3.7/wacom_sys.c b/3.7/wacom_sys.c
index 3047773..da086ed 100644
--- a/3.7/wacom_sys.c
+++ b/3.7/wacom_sys.c
@@ -1662,6 +1662,12 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
usb_make_path(dev, wacom->phys, sizeof(wacom->phys));
strlcat(wacom->phys, "/input0", sizeof(wacom->phys));
+ /* Verify that a device really has an endpoint */
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
+ error = -EINVAL;
+ goto fail3;
+ }
+
endpoint = &intf->cur_altsetting->endpoint[0].desc;
/* set the default size in case we do not get them from hid */
--
2.8.2
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Linuxwacom-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel