[PATCH 0/7] Input: fix NULL-derefs at probe

[PATCH 0/7] Input: fix NULL-derefs at probe

[Johan Hovold]

This series fixes a number of NULL-pointer dereferences due to missing
endpoint sanity checks that can be triggered by a malicious USB device.

Johan


Johan Hovold (7):
  Input: iforce - fix NULL-deref at probe
  Input: cm109 - fix NULL-deref at probe
  Input: ims-pcu - fix NULL-deref at probe
  Input: yealink - fix NULL-deref at probe
  Input: hanwang - fix NULL-deref at probe
  Input: kbtab - fix NULL-deref at probe
  Input: sur40 - fix NULL-deref at probe

 drivers/input/joystick/iforce/iforce-usb.c | 3 +++
 drivers/input/misc/cm109.c                 | 4 ++++
 drivers/input/misc/ims-pcu.c               | 4 ++++
 drivers/input/misc/yealink.c               | 4 ++++
 drivers/input/tablet/hanwang.c             | 3 +++
 drivers/input/tablet/kbtab.c               | 3 +++
 drivers/input/touchscreen/sur40.c          | 3 +++
 7 files changed, 24 insertions(+)

-- 
2.12.0

[PATCH 1/7] Input: iforce - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory that lie beyond the end of the endpoint
array should a malicious device lack the expected endpoints.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/joystick/iforce/iforce-usb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/joystick/iforce/iforce-usb.c b/drivers/input/joystick/iforce/iforce-usb.c
index d96aa27dfcdc..db64adfbe1af 100644
--- a/drivers/input/joystick/iforce/iforce-usb.c
+++ b/drivers/input/joystick/iforce/iforce-usb.c
@@ -141,6 +141,9 @@ static int iforce_usb_probe(struct usb_interface *intf,
 
 	interface = intf->cur_altsetting;
 
+	if (interface->desc.bNumEndpoints < 2)
+		return -ENODEV;
+
 	epirq = &interface->endpoint[0].desc;
 	epout = &interface->endpoint[1].desc;
 
-- 
2.12.0

[PATCH 3/7] Input: ims-pcu - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack control-interface endpoints.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable <stable@vger.kernel.org>	# 3.10
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/misc/ims-pcu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 9c0ea36913b4..f4e8fbec6a94 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1667,6 +1667,10 @@ static int ims_pcu_parse_cdc_data(struct usb_interface *intf, struct ims_pcu *pc
 		return -EINVAL;
 
 	alt = pcu->ctrl_intf->cur_altsetting;
+
+	if (alt->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	pcu->ep_ctrl = &alt->endpoint[0].desc;
 	pcu->max_ctrl_size = usb_endpoint_maxp(pcu->ep_ctrl);
 
-- 
2.12.0

[PATCH 4/7] Input: yealink - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack endpoints.

Fixes: aca951a22a1d ("[PATCH] input-driver-yealink-P1K-usb-phone")
Cc: stable <stable@vger.kernel.org>	# 2.6.14
Cc: Henk <Henk.Vergonet@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/misc/yealink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
index 79c964c075f1..6e7ff9561d92 100644
--- a/drivers/input/misc/yealink.c
+++ b/drivers/input/misc/yealink.c
@@ -875,6 +875,10 @@ static int usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	int ret, pipe, i;
 
 	interface = intf->cur_altsetting;
+
+	if (interface->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	endpoint = &interface->endpoint[0].desc;
 	if (!usb_endpoint_is_int_in(endpoint))
 		return -ENODEV;
-- 
2.12.0

[PATCH 5/7] Input: hanwang - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack endpoints.

Fixes: bba5394ad3bd ("Input: add support for Hanwang tablets")
Cc: stable <stable@vger.kernel.org>	# 2.6.37
Cc: Xing Wei <weixing@hanwang.com.cn>
Cc: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/tablet/hanwang.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/tablet/hanwang.c b/drivers/input/tablet/hanwang.c
index cd852059b99e..df4bea96d7ed 100644
--- a/drivers/input/tablet/hanwang.c
+++ b/drivers/input/tablet/hanwang.c
@@ -340,6 +340,9 @@ static int hanwang_probe(struct usb_interface *intf, const struct usb_device_id
 	int error;
 	int i;
 
+	if (intf->cur_altsetting->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	hanwang = kzalloc(sizeof(struct hanwang), GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!hanwang || !input_dev) {
-- 
2.12.0

[PATCH 6/7] Input: kbtab - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack endpoints.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/tablet/kbtab.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/tablet/kbtab.c b/drivers/input/tablet/kbtab.c
index e850d7e8afbc..4d9d64908b59 100644
--- a/drivers/input/tablet/kbtab.c
+++ b/drivers/input/tablet/kbtab.c
@@ -122,6 +122,9 @@ static int kbtab_probe(struct usb_interface *intf, const struct usb_device_id *i
 	struct input_dev *input_dev;
 	int error = -ENOMEM;
 
+	if (intf->cur_altsetting->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	kbtab = kzalloc(sizeof(struct kbtab), GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!kbtab || !input_dev)
-- 
2.12.0

[PATCH 7/7] Input: sur40 - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory that lie beyond the end of the endpoint
array should a malicious device lack the expected endpoints.

Fixes: bdb5c57f209c ("Input: add sur40 driver for Samsung SUR40 (aka MS
Surface 2.0/Pixelsense)")
Cc: stable <stable@vger.kernel.org>	# 3.13
Cc: Florian Echtler <floe@butterbrot.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Henrik Rydberg <rydberg@euromail.se>

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/touchscreen/sur40.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index aefb6e11f88a..4c0eecae065c 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -527,6 +527,9 @@ static int sur40_probe(struct usb_interface *interface,
 	if (iface_desc->desc.bInterfaceClass != 0xFF)
 		return -ENODEV;
 
+	if (iface_desc->desc.bNumEndpoints < 5)
+		return -ENODEV;
+
 	/* Use endpoint #4 (0x86). */
 	endpoint = &iface_desc->endpoint[4].desc;
 	if (endpoint->bEndpointAddress != TOUCH_ENDPOINT)
-- 
2.12.0

[PATCH 2/7] Input: cm109 - fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack endpoints.

Fixes: c04148f915e5 ("Input: add driver for USB VoIP phones with CM109
chipset")
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>	# 2.6.28
Cc: Alfred E. Heggestad <aeh-VqIxOk2P1jc@public.gmane.org>

Signed-off-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/input/misc/cm109.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
index 9cc6d057c302..23c191a2a071 100644
--- a/drivers/input/misc/cm109.c
+++ b/drivers/input/misc/cm109.c
@@ -700,6 +700,10 @@ static int cm109_usb_probe(struct usb_interface *intf,
 	int error = -ENOMEM;
 
 	interface = intf->cur_altsetting;
+
+	if (interface->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	endpoint = &interface->endpoint[0].desc;
 
 	if (!usb_endpoint_is_int_in(endpoint))
-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Oliver Neukum]

Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> This series fixes a number of NULL-pointer dereferences due to
> missing
> endpoint sanity checks that can be triggered by a malicious USB
> device.
> 
At the risk of repeating myself, doesn't the sheer number of fixes
demonstrate the need for a more centralized check?

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Johan Hovold]

On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > This series fixes a number of NULL-pointer dereferences due to
> > missing
> > endpoint sanity checks that can be triggered by a malicious USB
> > device.
>
> At the risk of repeating myself, doesn't the sheer number of fixes
> demonstrate the need for a more centralized check?

No, I don't think that follows. These are plain bugs that needs to be
fixed (cf. not checking for allocation failures or whatever) and
backported to the stable trees.

I think I may have surveyed just about every USB driver this last week,
and there is no single pattern for how endpoints are verified and
retrieved that could easily be refactored into USB core.

Now there are certain patterns that could benefit from a few helpers,
and some obvious bugs could then be caught by declaring those helpers as
__must_check. But specifically, you'd still be checking the return
value from the helpers.

Then verifying the endpoint counts before calling driver probe,
typically only saves a bit of time while probing *malicious* devices
(and the occasional odd interface which cannot be matched on other
attributes).

That being said, we could still add a centralised sanity check for a
large class of drivers (e.g. that do not use altsettings and only need
minimum constraints) but it's not going to obviate the need for careful
driver implementations.

I'll be posting some more patches related to this shortly.

Thanks,
Johan

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Dmitry Torokhov]

On Mon, Mar 13, 2017 at 04:45:52PM +0100, Johan Hovold wrote:
> On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> > Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > > This series fixes a number of NULL-pointer dereferences due to
> > > missing
> > > endpoint sanity checks that can be triggered by a malicious USB
> > > device.
> >
> > At the risk of repeating myself, doesn't the sheer number of fixes
> > demonstrate the need for a more centralized check?
> 
> No, I don't think that follows. These are plain bugs that needs to be
> fixed (cf. not checking for allocation failures or whatever) and
> backported to the stable trees.
> 
> I think I may have surveyed just about every USB driver this last week,
> and there is no single pattern for how endpoints are verified and
> retrieved that could easily be refactored into USB core.
> 
> Now there are certain patterns that could benefit from a few helpers,
> and some obvious bugs could then be caught by declaring those helpers as
> __must_check. But specifically, you'd still be checking the return
> value from the helpers.
> 
> Then verifying the endpoint counts before calling driver probe,
> typically only saves a bit of time while probing *malicious* devices
> (and the occasional odd interface which cannot be matched on other
> attributes).
> 
> That being said, we could still add a centralised sanity check for a
> large class of drivers (e.g. that do not use altsettings and only need
> minimum constraints) but it's not going to obviate the need for careful
> driver implementations.
> 
> I'll be posting some more patches related to this shortly.

There were some discussions about making and that would allow drivers
declare endpoints they want and have USB core ether fill them or not
even try to bind, but nothing concrete.

Anyway, I do not think we should be blocking this patch series on it; if
we come up with something clever we can always switch over.

Applied the lot.

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Johan Hovold]

On Thu, Mar 16, 2017 at 03:37:28PM -0700, Dmitry Torokhov wrote:
> On Mon, Mar 13, 2017 at 04:45:52PM +0100, Johan Hovold wrote:
> > On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> > > Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > > > This series fixes a number of NULL-pointer dereferences due to
> > > > missing endpoint sanity checks that can be triggered by a
> > > > malicious USB device.

> Applied the lot.

I noticed you dropped the Fixes tag from the patches that fix bugs which
predate git. While this is probably not much of an issue in this case, I
think it's generally a bad idea since we're loosing information this
way, and this specifically makes it harder for the stable maintainers to
figure out which tree to backport a fix to.

Thanks,
Johan

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Dmitry Torokhov]

On Fri, Mar 17, 2017 at 11:53:37AM +0100, Johan Hovold wrote:
> On Thu, Mar 16, 2017 at 03:37:28PM -0700, Dmitry Torokhov wrote:
> > On Mon, Mar 13, 2017 at 04:45:52PM +0100, Johan Hovold wrote:
> > > On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> > > > Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > > > > This series fixes a number of NULL-pointer dereferences due to
> > > > > missing endpoint sanity checks that can be triggered by a
> > > > > malicious USB device.
> 
> > Applied the lot.
> 
> I noticed you dropped the Fixes tag from the patches that fix bugs which
> predate git. While this is probably not much of an issue in this case, I
> think it's generally a bad idea since we're loosing information this
> way, and this specifically makes it harder for the stable maintainers to
> figure out which tree to backport a fix to.

As far as I know the rule is: if no special markings then stable patch
should be applied as far as it can go.

There is no reason to say specify 2.6.12 commit, as in fact the
offending change is likely to be even earlier, so the annotation would
be effectively wrong.

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Johan Hovold]

On Fri, Mar 17, 2017 at 02:03:15PM -0700, Dmitry Torokhov wrote:
> On Fri, Mar 17, 2017 at 11:53:37AM +0100, Johan Hovold wrote:
> > On Thu, Mar 16, 2017 at 03:37:28PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Mar 13, 2017 at 04:45:52PM +0100, Johan Hovold wrote:
> > > > On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> > > > > Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > > > > > This series fixes a number of NULL-pointer dereferences due to
> > > > > > missing endpoint sanity checks that can be triggered by a
> > > > > > malicious USB device.
> > 
> > > Applied the lot.
> > 
> > I noticed you dropped the Fixes tag from the patches that fix bugs which
> > predate git. While this is probably not much of an issue in this case, I
> > think it's generally a bad idea since we're loosing information this
> > way, and this specifically makes it harder for the stable maintainers to
> > figure out which tree to backport a fix to.
> 
> As far as I know the rule is: if no special markings then stable patch
> should be applied as far as it can go.

That's true for the stable tag itself, yes.

> There is no reason to say specify 2.6.12 commit, as in fact the
> offending change is likely to be even earlier, so the annotation would
> be effectively wrong.

It is still the first git commit which has the bug, and everyone
(dealing with code forensics) knows that 1da177e4c3f4
("Linux-2.6.12-rc2") is special.

Adding a Fixes-tag pointing to that initial commit, makes it clear that
bug has indeed been tracked as far back as reasonable. Omission of a
Fixes-tag could on the other hand be due to the submitter not bothering
to track the offending commit, thereby leaving it up to a stable
maintainer to do so (if only just be sure).

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html