[PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Johan Hovold]

The driver was checking the number of endpoints of the first alternate
setting instead of the current one, something which could be used by a
malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
dereference.

Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
Cc: stable <stable@vger.kernel.org>     # 4.8
Cc: Martin Kepplinger <martink@posteo.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/tablet/pegasus_notetaker.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index a1f3a0cb197e..38f087404f7a 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -275,7 +275,7 @@ static int pegasus_probe(struct usb_interface *intf,
 		return -ENODEV;
 
 	/* Sanity check that the device has an endpoint */
-	if (intf->altsetting[0].desc.bNumEndpoints < 1) {
+	if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
 		dev_err(&intf->dev, "Invalid number of endpoints\n");
 		return -EINVAL;
 	}
-- 
2.24.0

Re: [PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Martin Kepplinger]

On 10.12.19 12:37, Johan Hovold wrote:
> The driver was checking the number of endpoints of the first alternate
> setting instead of the current one, something which could be used by a
> malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
> dereference.
> 
> Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
> Cc: stable <stable@vger.kernel.org>     # 4.8
> Cc: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/input/tablet/pegasus_notetaker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index a1f3a0cb197e..38f087404f7a 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -275,7 +275,7 @@ static int pegasus_probe(struct usb_interface *intf,
>  		return -ENODEV;
>  
>  	/* Sanity check that the device has an endpoint */
> -	if (intf->altsetting[0].desc.bNumEndpoints < 1) {
> +	if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
>  		dev_err(&intf->dev, "Invalid number of endpoints\n");
>  		return -EINVAL;
>  	}
> 

Acked-by: Martin Kepplinger <martink@posteo.de>

Re: [PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Johan Hovold]

On Tue, Dec 10, 2019 at 12:37:31PM +0100, Johan Hovold wrote:
> The driver was checking the number of endpoints of the first alternate
> setting instead of the current one, something which could be used by a
> malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
> dereference.
> 
> Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
> Cc: stable <stable@vger.kernel.org>     # 4.8
> Cc: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Looks like the stable tag was removed when this one was applied, and
similar for patches 2, 4 and 7 of this series (commits 3111491fca4f,
a8eeb74df5a6, 6b32391ed675 upstream).

While the last three are mostly an issue for the syzbot fuzzer, we have
started backporting those as well.

This one (bcfcb7f9b480) is more clear cut as it can be used to trigger a
NULL-deref.

I only noticed because Sasha picked up one of the other patches in the
series which was never intended for stable.

> ---
>  drivers/input/tablet/pegasus_notetaker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index a1f3a0cb197e..38f087404f7a 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -275,7 +275,7 @@ static int pegasus_probe(struct usb_interface *intf,
>  		return -ENODEV;
>  
>  	/* Sanity check that the device has an endpoint */
> -	if (intf->altsetting[0].desc.bNumEndpoints < 1) {
> +	if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
>  		dev_err(&intf->dev, "Invalid number of endpoints\n");
>  		return -EINVAL;
>  	}

Johan

Re: [PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Greg KH]

On Tue, Feb 04, 2020 at 09:24:41AM +0100, Johan Hovold wrote:
> On Tue, Dec 10, 2019 at 12:37:31PM +0100, Johan Hovold wrote:
> > The driver was checking the number of endpoints of the first alternate
> > setting instead of the current one, something which could be used by a
> > malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
> > dereference.
> > 
> > Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
> > Cc: stable <stable@vger.kernel.org>     # 4.8
> > Cc: Martin Kepplinger <martink@posteo.de>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Looks like the stable tag was removed when this one was applied, and
> similar for patches 2, 4 and 7 of this series (commits 3111491fca4f,
> a8eeb74df5a6, 6b32391ed675 upstream).
> 
> While the last three are mostly an issue for the syzbot fuzzer, we have
> started backporting those as well.
> 
> This one (bcfcb7f9b480) is more clear cut as it can be used to trigger a
> NULL-deref.
> 
> I only noticed because Sasha picked up one of the other patches in the
> series which was never intended for stable.

Did I end up catching all of these properly?  I've had to expand my
search for some patches like this that do not explicitly have the cc:
stable mark on them as not all subsystems do this well (if at all.)

And there's also Sasha's work in digging up patches based on patterns of
fixes, which also is needed because of this "problem".

thanks,

greg k-h

Re: [PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Johan Hovold]

On Tue, Feb 04, 2020 at 10:02:32AM +0000, Greg Kroah-Hartman wrote:
> On Tue, Feb 04, 2020 at 09:24:41AM +0100, Johan Hovold wrote:
> > On Tue, Dec 10, 2019 at 12:37:31PM +0100, Johan Hovold wrote:
> > > The driver was checking the number of endpoints of the first alternate
> > > setting instead of the current one, something which could be used by a
> > > malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
> > > dereference.
> > > 
> > > Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
> > > Cc: stable <stable@vger.kernel.org>     # 4.8
> > > Cc: Martin Kepplinger <martink@posteo.de>
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > 
> > Looks like the stable tag was removed when this one was applied, and
> > similar for patches 2, 4 and 7 of this series (commits 3111491fca4f,
> > a8eeb74df5a6, 6b32391ed675 upstream).
> > 
> > While the last three are mostly an issue for the syzbot fuzzer, we have
> > started backporting those as well.
> > 
> > This one (bcfcb7f9b480) is more clear cut as it can be used to trigger a
> > NULL-deref.
> > 
> > I only noticed because Sasha picked up one of the other patches in the
> > series which was never intended for stable.
> 
> Did I end up catching all of these properly?  I've had to expand my
> search for some patches like this that do not explicitly have the cc:
> stable mark on them as not all subsystems do this well (if at all.)

No, sorry, should have been more clear on that point; these four were
never picked up for stable it seems.

I was a bit surprised to see the stable-tags be removed from the
original submissions here, even if I know the net-maintainers do this
routinely, and any maintainer can of course override a submitters
judgement.

> And there's also Sasha's work in digging up patches based on patterns of
> fixes, which also is needed because of this "problem".

Yeah, seems likely that autosel would have caught these eventually.

Johan

Re: [PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Dmitry Torokhov]

On Tue, Feb 04, 2020 at 11:14:35AM +0100, Johan Hovold wrote:
> On Tue, Feb 04, 2020 at 10:02:32AM +0000, Greg Kroah-Hartman wrote:
> > On Tue, Feb 04, 2020 at 09:24:41AM +0100, Johan Hovold wrote:
> > > On Tue, Dec 10, 2019 at 12:37:31PM +0100, Johan Hovold wrote:
> > > > The driver was checking the number of endpoints of the first alternate
> > > > setting instead of the current one, something which could be used by a
> > > > malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
> > > > dereference.
> > > > 
> > > > Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
> > > > Cc: stable <stable@vger.kernel.org>     # 4.8
> > > > Cc: Martin Kepplinger <martink@posteo.de>
> > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > 
> > > Looks like the stable tag was removed when this one was applied, and
> > > similar for patches 2, 4 and 7 of this series (commits 3111491fca4f,
> > > a8eeb74df5a6, 6b32391ed675 upstream).
> > > 
> > > While the last three are mostly an issue for the syzbot fuzzer, we have
> > > started backporting those as well.
> > > 
> > > This one (bcfcb7f9b480) is more clear cut as it can be used to trigger a
> > > NULL-deref.
> > > 
> > > I only noticed because Sasha picked up one of the other patches in the
> > > series which was never intended for stable.
> > 
> > Did I end up catching all of these properly?  I've had to expand my
> > search for some patches like this that do not explicitly have the cc:
> > stable mark on them as not all subsystems do this well (if at all.)
> 
> No, sorry, should have been more clear on that point; these four were
> never picked up for stable it seems.
> 
> I was a bit surprised to see the stable-tags be removed from the
> original submissions here, even if I know the net-maintainers do this
> routinely, and any maintainer can of course override a submitters
> judgement.

Sorry, dropping stable tags was not intentional. I was trying to improve
my tooling and drop the CC noise from the changelogs, but my script got
too eager and removed "Cc: stable" as well. I believe my tool should be
fixed now and it should not happen again.

Thanks.

-- 
Dmitry

Re: [PATCH 1/7] Input: pegasus_notetaker: fix endpoint sanity check

[Johan Hovold]

On Tue, Feb 04, 2020 at 11:24:34AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 04, 2020 at 11:14:35AM +0100, Johan Hovold wrote:
> > On Tue, Feb 04, 2020 at 10:02:32AM +0000, Greg Kroah-Hartman wrote:
> > > On Tue, Feb 04, 2020 at 09:24:41AM +0100, Johan Hovold wrote:
> > > > On Tue, Dec 10, 2019 at 12:37:31PM +0100, Johan Hovold wrote:
> > > > > The driver was checking the number of endpoints of the first alternate
> > > > > setting instead of the current one, something which could be used by a
> > > > > malicious device (or USB descriptor fuzzer) to trigger a NULL-pointer
> > > > > dereference.
> > > > > 
> > > > > Fixes: 1afca2b66aac ("Input: add Pegasus Notetaker tablet driver")
> > > > > Cc: stable <stable@vger.kernel.org>     # 4.8
> > > > > Cc: Martin Kepplinger <martink@posteo.de>
> > > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > 
> > > > Looks like the stable tag was removed when this one was applied, and
> > > > similar for patches 2, 4 and 7 of this series (commits 3111491fca4f,
> > > > a8eeb74df5a6, 6b32391ed675 upstream).
> > > > 
> > > > While the last three are mostly an issue for the syzbot fuzzer, we have
> > > > started backporting those as well.
> > > > 
> > > > This one (bcfcb7f9b480) is more clear cut as it can be used to trigger a
> > > > NULL-deref.
> > > > 
> > > > I only noticed because Sasha picked up one of the other patches in the
> > > > series which was never intended for stable.
> > > 
> > > Did I end up catching all of these properly?  I've had to expand my
> > > search for some patches like this that do not explicitly have the cc:
> > > stable mark on them as not all subsystems do this well (if at all.)
> > 
> > No, sorry, should have been more clear on that point; these four were
> > never picked up for stable it seems.
> > 
> > I was a bit surprised to see the stable-tags be removed from the
> > original submissions here, even if I know the net-maintainers do this
> > routinely, and any maintainer can of course override a submitters
> > judgement.
> 
> Sorry, dropping stable tags was not intentional. I was trying to improve
> my tooling and drop the CC noise from the changelogs, but my script got
> too eager and removed "Cc: stable" as well. I believe my tool should be
> fixed now and it should not happen again.

Ah, ok, thanks for confirming.

Johan

[PATCH 2/7] Input: aiptek: fix endpoint sanity check

[Johan Hovold]

The driver was checking the number of endpoints of the first alternate
setting instead of the current one, something which could lead to the
driver binding to an invalid interface.

This in turn could cause the driver to misbehave or trigger a WARN() in
usb_submit_urb() that kernels with panic_on_warn set would choke on.

Fixes: 8e20cf2bce12 ("Input: aiptek - fix crash on detecting device without endpoints")
Cc: stable <stable@vger.kernel.org>     # 4.4
Cc: Vladis Dronov <vdronov@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/tablet/aiptek.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
index 2ca586fb914f..06d0ffef4a17 100644
--- a/drivers/input/tablet/aiptek.c
+++ b/drivers/input/tablet/aiptek.c
@@ -1802,14 +1802,14 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	input_set_abs_params(inputdev, ABS_WHEEL, AIPTEK_WHEEL_MIN, AIPTEK_WHEEL_MAX - 1, 0, 0);
 
 	/* Verify that a device really has an endpoint */
-	if (intf->altsetting[0].desc.bNumEndpoints < 1) {
+	if (intf->cur_altsetting->desc.bNumEndpoints < 1) {
 		dev_err(&intf->dev,
 			"interface has %d endpoints, but must have minimum 1\n",
-			intf->altsetting[0].desc.bNumEndpoints);
+			intf->cur_altsetting->desc.bNumEndpoints);
 		err = -EINVAL;
 		goto fail3;
 	}
-	endpoint = &intf->altsetting[0].endpoint[0].desc;
+	endpoint = &intf->cur_altsetting->endpoint[0].desc;
 
 	/* Go set up our URB, which is called when the tablet receives
 	 * input.
-- 
2.24.0

Re: [PATCH 2/7] Input: aiptek: fix endpoint sanity check

[Vladis Dronov]

A fix for a bug indeed, thank you Johan.

Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer

----- Original Message -----
> From: "Johan Hovold" <johan@kernel.org>
> To: "Dmitry Torokhov" <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, "Johan Hovold"
> <johan@kernel.org>, "stable" <stable@vger.kernel.org>, "Vladis Dronov" <vdronov@redhat.com>
> Sent: Tuesday, December 10, 2019 12:37:32 PM
> Subject: [PATCH 2/7] Input: aiptek: fix endpoint sanity check
> 
> The driver was checking the number of endpoints of the first alternate
> setting instead of the current one, something which could lead to the
> driver binding to an invalid interface.
> 
> This in turn could cause the driver to misbehave or trigger a WARN() in
> usb_submit_urb() that kernels with panic_on_warn set would choke on.
> 
> Fixes: 8e20cf2bce12 ("Input: aiptek - fix crash on detecting device without
> endpoints")
> Cc: stable <stable@vger.kernel.org>     # 4.4
> Cc: Vladis Dronov <vdronov@redhat.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

[PATCH 4/7] Input: gtco: fix endpoint sanity check

[Johan Hovold]

The driver was checking the number of endpoints of the first alternate
setting instead of the current one, something which could lead to the
driver binding to an invalid interface.

This in turn could cause the driver to misbehave or trigger a WARN() in
usb_submit_urb() that kernels with panic_on_warn set would choke on.

Fixes: 162f98dea487 ("Input: gtco - fix crash on detecting device without endpoints")
Cc: stable <stable@vger.kernel.org>     # 4.6
Cc: Vladis Dronov <vdronov@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/tablet/gtco.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
index 35031228a6d0..799c94dda651 100644
--- a/drivers/input/tablet/gtco.c
+++ b/drivers/input/tablet/gtco.c
@@ -875,18 +875,14 @@ static int gtco_probe(struct usb_interface *usbinterface,
 	}
 
 	/* Sanity check that a device has an endpoint */
-	if (usbinterface->altsetting[0].desc.bNumEndpoints < 1) {
+	if (usbinterface->cur_altsetting->desc.bNumEndpoints < 1) {
 		dev_err(&usbinterface->dev,
 			"Invalid number of endpoints\n");
 		error = -EINVAL;
 		goto err_free_urb;
 	}
 
-	/*
-	 * The endpoint is always altsetting 0, we know this since we know
-	 * this device only has one interrupt endpoint
-	 */
-	endpoint = &usbinterface->altsetting[0].endpoint[0].desc;
+	endpoint = &usbinterface->cur_altsetting->endpoint[0].desc;
 
 	/* Some debug */
 	dev_dbg(&usbinterface->dev, "gtco # interfaces: %d\n", usbinterface->num_altsetting);
@@ -973,7 +969,7 @@ static int gtco_probe(struct usb_interface *usbinterface,
 	input_dev->dev.parent = &usbinterface->dev;
 
 	/* Setup the URB, it will be posted later on open of input device */
-	endpoint = &usbinterface->altsetting[0].endpoint[0].desc;
+	endpoint = &usbinterface->cur_altsetting->endpoint[0].desc;
 
 	usb_fill_int_urb(gtco->urbinfo,
 			 udev,
-- 
2.24.0

Re: [PATCH 4/7] Input: gtco: fix endpoint sanity check

[Vladis Dronov]

Thanks, Johan! A fix for a bug indeed.

Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer

----- Original Message -----
> From: "Johan Hovold" <johan@kernel.org>
> To: "Dmitry Torokhov" <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, "Johan Hovold"
> <johan@kernel.org>, "stable" <stable@vger.kernel.org>, "Vladis Dronov" <vdronov@redhat.com>
> Sent: Tuesday, December 10, 2019 12:37:34 PM
> Subject: [PATCH 4/7] Input: gtco: fix endpoint sanity check
> 
> The driver was checking the number of endpoints of the first alternate
> setting instead of the current one, something which could lead to the
> driver binding to an invalid interface.
> 
> This in turn could cause the driver to misbehave or trigger a WARN() in
> usb_submit_urb() that kernels with panic_on_warn set would choke on.
> 
> Fixes: 162f98dea487 ("Input: gtco - fix crash on detecting device without
> endpoints")
> Cc: stable <stable@vger.kernel.org>     # 4.6
> Cc: Vladis Dronov <vdronov@redhat.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

[PATCH 7/7] Input: sur40: fix interface sanity checks

[Johan Hovold]

Make sure to use the current alternate setting when verifying the
interface descriptors to avoid binding to an invalid interface.

This in turn could cause the driver to misbehave or trigger a WARN() in
usb_submit_urb() that kernels with panic_on_warn set would choke on.

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>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/touchscreen/sur40.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 1dd47dda71cd..34d31c7ec8ba 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -661,7 +661,7 @@ static int sur40_probe(struct usb_interface *interface,
 	int error;
 
 	/* Check if we really have the right interface. */
-	iface_desc = &interface->altsetting[0];
+	iface_desc = interface->cur_altsetting;
 	if (iface_desc->desc.bInterfaceClass != 0xFF)
 		return -ENODEV;
 
-- 
2.24.0