[PATCH] Input: aiptek: fix crash on detecting device without endpoints

[PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Vladis Dronov]

The aiptek driver crashes in aiptek_probe() when a specially crafted usb device
without endpoints is detected. This fix adds a check that the device has proper
configuration expected by the driver. Also an error return value is changed to
more matching one in one of the error paths.

Reported-by: Ralf Spenneberg <ralf@spenneberg.net>
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 drivers/input/tablet/aiptek.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
index e7f966d..78c0732 100644
--- a/drivers/input/tablet/aiptek.c
+++ b/drivers/input/tablet/aiptek.c
@@ -1819,6 +1819,15 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	input_set_abs_params(inputdev, ABS_TILT_Y, AIPTEK_TILT_MIN, AIPTEK_TILT_MAX, 0, 0);
 	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) {
+		dev_warn(&intf->dev,
+			"interface has %d endpoints, but must have minimum 1\n",
+			intf->altsetting[0].desc.bNumEndpoints);
+		err = -ENODEV;
+		goto fail3;
+	}
 	endpoint = &intf->altsetting[0].endpoint[0].desc;
 
 	/* Go set up our URB, which is called when the tablet receives
@@ -1861,6 +1870,7 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	if (i == ARRAY_SIZE(speeds)) {
 		dev_info(&intf->dev,
 			 "Aiptek tried all speeds, no sane response\n");
+		err = -ENODEV;
 		goto fail3;
 	}
 
-- 
2.6.2

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Dmitry Torokhov]

Hi Vladis,

On Wed, Nov 25, 2015 at 04:58:08PM +0100, Vladis Dronov wrote:
> The aiptek driver crashes in aiptek_probe() when a specially crafted usb device
> without endpoints is detected. This fix adds a check that the device has proper
> configuration expected by the driver. Also an error return value is changed to
> more matching one in one of the error paths.

Hmm, I see quite a few drivers assuming that endpoint 0 will be present.
I wonder if that should not be solved at USB level.

Alan, does it make sense to have drivers probe interface if it does not
have any endpoints?

Thanks.

> 
> Reported-by: Ralf Spenneberg <ralf@spenneberg.net>
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
>  drivers/input/tablet/aiptek.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
> index e7f966d..78c0732 100644
> --- a/drivers/input/tablet/aiptek.c
> +++ b/drivers/input/tablet/aiptek.c
> @@ -1819,6 +1819,15 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	input_set_abs_params(inputdev, ABS_TILT_Y, AIPTEK_TILT_MIN, AIPTEK_TILT_MAX, 0, 0);
>  	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) {
> +		dev_warn(&intf->dev,
> +			"interface has %d endpoints, but must have minimum 1\n",
> +			intf->altsetting[0].desc.bNumEndpoints);
> +		err = -ENODEV;
> +		goto fail3;
> +	}
>  	endpoint = &intf->altsetting[0].endpoint[0].desc;
>  
>  	/* Go set up our URB, which is called when the tablet receives
> @@ -1861,6 +1870,7 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	if (i == ARRAY_SIZE(speeds)) {
>  		dev_info(&intf->dev,
>  			 "Aiptek tried all speeds, no sane response\n");
> +		err = -ENODEV;
>  		goto fail3;
>  	}
>  
> -- 
> 2.6.2
> 

-- 
Dmitry

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Alan Stern]

On Thu, 26 Nov 2015, Dmitry Torokhov wrote:

> Hi Vladis,
> 
> On Wed, Nov 25, 2015 at 04:58:08PM +0100, Vladis Dronov wrote:
> > The aiptek driver crashes in aiptek_probe() when a specially crafted usb device
> > without endpoints is detected. This fix adds a check that the device has proper
> > configuration expected by the driver. Also an error return value is changed to
> > more matching one in one of the error paths.
> 
> Hmm, I see quite a few drivers assuming that endpoint 0 will be present.
> I wonder if that should not be solved at USB level.

Every USB device always has endpoint 0.  If one didn't, the kernel 
wouldn't be able to initialize and enumerate it.

> Alan, does it make sense to have drivers probe interface if it does not
> have any endpoints?

Yes; in theory an interface can do everything it needs using only 
endpoint 0.  Driver shouldn't assume anything about the endpoints in 
the interfaces they problem.

Alan Stern

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Vladis Dronov]

Hello,

> > Hmm, I see quite a few drivers assuming that endpoint 0 will be present.
> > I wonder if that should not be solved at USB level.
>
> Every USB device always has endpoint 0.  If one didn't, the kernel 
> wouldn't be able to initialize and enumerate it.

Yes for the normal USB device. This case is a weird USB device (with no
endpoints) specially designed to be weird. My point here is that even a
weird device probing should not crash a kernel by a NULL-deref.

> > Alan, does it make sense to have drivers probe interface if it does not
> > have any endpoints?
>
> Yes; in theory an interface can do everything it needs using only 
> endpoint 0.  Driver shouldn't assume anything about the endpoints in 
> the interfaces they problem.

The current kernel code in drivers/usb/core/config.c accepts an interface
with no endpoints in one of its configurations, and I could not find a
direct ban for that in USB standard. So, I currently believe, it is a driver
job to check if the endpoint 0 exist, otherwise we must change the kernel
USB detection code.

btw, indeed, this is not the only driver with this problem, I've met 2 more.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Alan Stern]

On Fri, 27 Nov 2015, Vladis Dronov wrote:

> Hello,
> 
> > > Hmm, I see quite a few drivers assuming that endpoint 0 will be present.
> > > I wonder if that should not be solved at USB level.
> >
> > Every USB device always has endpoint 0.  If one didn't, the kernel 
> > wouldn't be able to initialize and enumerate it.
> 
> Yes for the normal USB device. This case is a weird USB device (with no
> endpoints) specially designed to be weird. My point here is that even a
> weird device probing should not crash a kernel by a NULL-deref.

True.  However, a weird device that didn't have any endpoint 0 would
not crash the kernel, because the kernel wouldn't be able to
communicate with it at all.

> > > Alan, does it make sense to have drivers probe interface if it does not
> > > have any endpoints?
> >
> > Yes; in theory an interface can do everything it needs using only 
> > endpoint 0.  Driver shouldn't assume anything about the endpoints in 
> > the interfaces they problem.
> 
> The current kernel code in drivers/usb/core/config.c accepts an interface
> with no endpoints in one of its configurations, and I could not find a
> direct ban for that in USB standard. So, I currently believe, it is a driver
> job to check if the endpoint 0 exist, otherwise we must change the kernel
> USB detection code.

Drivers do not have to check whether endpoint 0 exists; it always does.  
But they do have to check any other endpoint they use.

> btw, indeed, this is not the only driver with this problem, I've met 2 more.

In fact, there's no way to check whether endpoint 0 exists.  Where 
would you look to find out?  There isn't any endpoint descriptor for 
it.

Alan Stern

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Vladis Dronov]

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]

Hello, Alan, all.

> > > > Hmm, I see quite a few drivers assuming that endpoint 0 will be present.
> > > > I wonder if that should not be solved at USB level.
> > >
> > > Every USB device always has endpoint 0.  If one didn't, the kernel 
> > > wouldn't be able to initialize and enumerate it.
> > 
> > Yes for the normal USB device. This case is a weird USB device (with no
> > endpoints) specially designed to be weird. My point here is that even a
> > weird device probing should not crash a kernel by a NULL-deref.
> 
> True.  However, a weird device that didn't have any endpoint 0 would
> not crash the kernel, because the kernel wouldn't be able to
> communicate with it at all.

I'm afraid, I do not get you here. A device in question _does_ crash the kernel
(or driver only) as this call trace shows (see attached for the full call trace),
and that's why the patch is proposed:

[  622.444650] BUG: unable to handle kernel NULL pointer dereference at 0000000000000002
[  622.445019] IP: [<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
[  622.445019] RIP: 0010:[<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
[  622.445019] RAX: 0000000000000000 RBX: ffff88000bd67800 RCX: ffff88000bcd0800
                        ^^^ rax is NULL, it is being dereferenced

Kernel does not communicates with the device, but the driver tries to access
a kernel structure, which was not allocated (thus, a null-deref):

endpoint = &intf->altsetting[0].endpoint[0].desc; // the structure endpoint[0]
                                             // was not kzalloc()ed, thus NULL
usb_rcvintpipe(aiptek->usbdev, endpoint->bEndpointAddress) // NULL deref

> > > > Alan, does it make sense to have drivers probe interface if it does not
> > > > have any endpoints?
> > >
> > > Yes; in theory an interface can do everything it needs using only 
> > > endpoint 0.  Driver shouldn't assume anything about the endpoints in 
> > > the interfaces they problem.
> > 
> > The current kernel code in drivers/usb/core/config.c accepts an interface
> > with no endpoints in one of its configurations, and I could not find a
> > direct ban for that in USB standard. So, I currently believe, it is a driver
> > job to check if the endpoint 0 exist, otherwise we must change the kernel
> > USB detection code.
> 
> Drivers do not have to check whether endpoint 0 exists; it always does.  
> But they do have to check any other endpoint they use.

As the above example shows, endpoint 0 does not always exists. A specially
crafted weird USB device can advertise absence of endpoint 0 and the kernel
will accept this:

[drivers/usb/core/config.c]
num_ep = num_ep_orig = alt->desc.bNumEndpoints;  // weird device can have 0 here
alt->desc.bNumEndpoints = 0;
if (num_ep > 0) {
        /* Can't allocate 0 bytes */
        len = sizeof(struct usb_host_endpoint) * num_ep;
        alt->endpoint = kzalloc(len, GFP_KERNEL);

As far as I understand, this is a question we discuss here: whose responsibility
is to handle situations of endpoint 0 absence in an altconfig?

- if it is driver's job, a driver much check this, as in the patch I propose

- if it is a kernel job not to allow devices with no endpoint 0 in one the
configurations, the current USB device detection implementation (in
drivers/usb/core/config.c) should be modified, as currently it allows such.

I do not have much expertise in the Linux USB stack, so I'm asking you and
the community for an advise on this.

> > btw, indeed, this is not the only driver with this problem, I've met 2 more.
> 
> In fact, there's no way to check whether endpoint 0 exists.  Where 
> would you look to find out?  There isn't any endpoint descriptor for it.

I believe, there is a configuration descriptor for that, probably, I'm wrong:

if (intf->altsetting[0].desc.bNumEndpoints < 1) { ...

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

[-- Attachment #2: stacktrace.txt --]
[-- Type: text/plain, Size: 4678 bytes --]

[  622.149957] usb 1-1: new full-speed USB device number 2 using xhci_hcd
[  622.354485] usb 1-1: config 1 interface 0 altsetting 0 has 3 endpoint descriptors, different from the interface descriptor's value: 0
[  622.386630] usb 1-1: New USB device found, idVendor=0458, idProduct=5003
[  622.392414] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  622.399416] usb 1-1: Product: ĉ
[  622.404640] usb 1-1: Manufacturer: ĉ
[  622.410079] usb 1-1: SerialNumber: %
[  622.444650] BUG: unable to handle kernel NULL pointer dereference at 0000000000000002
[  622.445019] IP: [<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
[  622.445019] PGD 0 
[  622.445019] Oops: 0000 [#1] SMP 
[  622.445019] Modules linked in: aiptek(+) ip6t_rpfilter ip6t_REJECT ipt_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw iptable_filter ip_tables bochs_drm ppdev syscopyarea sysfillrect sysimgblt ttm drm_kms_helper drm pcspkr i2c_piix4 i2c_core serio_raw parport_pc parport xfs libcrc32c sd_mod sr_mod crc_t10dif cdrom crct10dif_common ata_generic pata_acpi ata_piix libata e1000 floppy dm_mirror dm_region_hash dm_log dm_mod
[  622.445019] CPU: 0 PID: 2242 Comm: systemd-udevd Not tainted 3.10.0-229.14.1.el7.x86_64 #1
[  622.445019] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
[  622.445019] task: ffff88000e65a220 ti: ffff88000f4cc000 task.ti: ffff88000f4cc000
[  622.445019] RIP: 0010:[<ffffffffa0395303>]  [<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
[  622.445019] RSP: 0018:ffff88000f4cfb80  EFLAGS: 00010286
[  622.445019] RAX: 0000000000000000 RBX: ffff88000bd67800 RCX: ffff88000bcd0800
[  622.445019] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffff88000ca29000
[  622.445019] RBP: ffff88000f4cfbe0 R08: 0000000000000000 R09: 0000000000000000
[  622.445019] R10: ffff88000e401400 R11: ffffffff810020d8 R12: ffff88000c525800
[  622.445019] R13: ffff88000c525830 R14: ffff88000bcd1800 R15: ffff88000bd67834
[  622.445019] FS:  00007fb8082b4880(0000) GS:ffff88000fc00000(0000) knlGS:0000000000000000
[  622.445019] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  622.445019] CR2: 0000000000000002 CR3: 000000000d67f000 CR4: 00000000000006f0
[  622.445019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  622.445019] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  622.445019] Stack:
[  622.445019]  ffff88000bcd0800 0000000000000001 0000019000000246 0000019000000032
[  622.445019]  0000006400000019 0000012c000000c8 000000000cc3e092 ffff88000bcd0890
[  622.445019]  ffff88000bcd0800 ffffffffa0397068 ffff88000c525830 ffffffffa03965c0
[  622.445019] Call Trace:
[  622.445019]  [<ffffffff8141dc04>] usb_probe_interface+0x1c4/0x2f0
[  622.445019]  [<ffffffff813d30d7>] driver_probe_device+0x87/0x390
[  622.445019]  [<ffffffff813d34b3>] __driver_attach+0x93/0xa0
[  622.445019]  [<ffffffff813d3420>] ? __device_attach+0x40/0x40
[  622.445019]  [<ffffffff813d0e43>] bus_for_each_dev+0x73/0xc0
[  622.445019]  [<ffffffff813d2b2e>] driver_attach+0x1e/0x20
[  622.445019]  [<ffffffff813d2680>] bus_add_driver+0x200/0x2d0
[  622.445019]  [<ffffffff813d3b34>] driver_register+0x64/0xf0
[  622.445019]  [<ffffffff8141c1c2>] usb_register_driver+0x82/0x160
[  622.445019]  [<ffffffffa039a000>] ? 0xffffffffa0399fff
[  622.445019]  [<ffffffffa039a01e>] aiptek_driver_init+0x1e/0x1000 [aiptek]
[  622.445019]  [<ffffffff810020e8>] do_one_initcall+0xb8/0x230
[  622.445019]  [<ffffffff810dd0ee>] load_module+0x133e/0x1b40
[  622.445019]  [<ffffffff812f7d60>] ? ddebug_proc_write+0xf0/0xf0
[  622.445019]  [<ffffffff810d96b3>] ? copy_module_from_fd.isra.42+0x53/0x150
[  622.445019]  [<ffffffff810ddaa6>] SyS_finit_module+0xa6/0xd0
[  622.445019]  [<ffffffff81614389>] system_call_fastpath+0x16/0x1b
[  622.445019] Code: 45 31 c9 45 31 c0 b9 ff 03 00 00 be 08 00 00 00 4c 89 f7 e8 90 39 0d e1 49 8b 04 24 48 8b 4b 08 48 8b bb 10 01 00 00 48 8b 40 18 <0f> b6 50 02 0f b6 70 06 8b 01 c1 e2 0f c1 e0 08 81 ca 80 00 00 
[  622.445019] RIP  [<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
[  622.445019]  RSP <ffff88000f4cfb80>
[  622.445019] CR2: 0000000000000002
[  622.860772] ---[ end trace b239663354a1c556 ]---
[  622.864813] Kernel panic - not syncing: Fatal exception
[  622.865768] drm_kms_helper: panic occurred, switching back to text console

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Alan Stern]

On Mon, 30 Nov 2015, Vladis Dronov wrote:

> Hello, Alan, all.

Hello.

> > > Yes for the normal USB device. This case is a weird USB device (with no
> > > endpoints) specially designed to be weird. My point here is that even a
> > > weird device probing should not crash a kernel by a NULL-deref.
> > 
> > True.  However, a weird device that didn't have any endpoint 0 would
> > not crash the kernel, because the kernel wouldn't be able to
> > communicate with it at all.
> 
> I'm afraid, I do not get you here. A device in question _does_ crash the kernel
> (or driver only) as this call trace shows (see attached for the full call trace),
> and that's why the patch is proposed:
> 
> [  622.444650] BUG: unable to handle kernel NULL pointer dereference at 0000000000000002
> [  622.445019] IP: [<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
> [  622.445019] RIP: 0010:[<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
> [  622.445019] RAX: 0000000000000000 RBX: ffff88000bd67800 RCX: ffff88000bcd0800
>                         ^^^ rax is NULL, it is being dereferenced

This is the result of a bug in the aiptek driver.  See below.

> Kernel does not communicates with the device,

Yes it does.  Otherwise how would the kernel know that the aiptek
driver was the one which should be probed for this device?

>  but the driver tries to access
> a kernel structure, which was not allocated (thus, a null-deref):
> 
> endpoint = &intf->altsetting[0].endpoint[0].desc; // the structure endpoint[0]
>                                              // was not kzalloc()ed, thus NULL
> usb_rcvintpipe(aiptek->usbdev, endpoint->bEndpointAddress) // NULL deref

Right.  That's the bug; aiptek should not try to access a data 
structure that was never allocated.

> > Drivers do not have to check whether endpoint 0 exists; it always does.  
> > But they do have to check any other endpoint they use.
> 
> As the above example shows, endpoint 0 does not always exists. A specially
> crafted weird USB device can advertise absence of endpoint 0 and the kernel
> will accept this:

The example above has nothing to do with endpoint 0.  The entries in
the altsetting[n].endpoint array are not stored in numerical order;  
entry 0 in this array does not refer to endpoint 0.  (See the comment
for the endpoint field in the definition of struct usb_host_interface
in include/linux/usb.h.)  In fact, endpoint 0 is treated specially and
isn't part of this array at all.

> [drivers/usb/core/config.c]
> num_ep = num_ep_orig = alt->desc.bNumEndpoints;  // weird device can have 0 here
> alt->desc.bNumEndpoints = 0;
> if (num_ep > 0) {
>         /* Can't allocate 0 bytes */
>         len = sizeof(struct usb_host_endpoint) * num_ep;
>         alt->endpoint = kzalloc(len, GFP_KERNEL);
> 
> As far as I understand, this is a question we discuss here: whose responsibility
> is to handle situations of endpoint 0 absence in an altconfig?

num_ep counts the number of endpoints _other_ than endpoint 0.  
There's no point counting endpoint 0 because it is always present.  
Not to mention that it doesn't get stored in this array, since endpoint
0 has no descriptor.

> - if it is driver's job, a driver much check this, as in the patch I propose
> 
> - if it is a kernel job not to allow devices with no endpoint 0 in one the
> configurations, the current USB device detection implementation (in
> drivers/usb/core/config.c) should be modified, as currently it allows such.
> 
> I do not have much expertise in the Linux USB stack, so I'm asking you and
> the community for an advise on this.

My advice is to fix aiptek's probe routine.  It should check that 
intf->altsetting[0].desc.bNumEndpoints > 0 before trying to accessing 
the endpoint array.

> > > btw, indeed, this is not the only driver with this problem, I've met 2 more.
> > 
> > In fact, there's no way to check whether endpoint 0 exists.  Where 
> > would you look to find out?  There isn't any endpoint descriptor for it.
> 
> I believe, there is a configuration descriptor for that, probably, I'm wrong:
> 
> if (intf->altsetting[0].desc.bNumEndpoints < 1) { ...

That value is the number of endpoints _other_ than endpoint 0.  This is 
documented in the USB-2.0 specification, Table 9-12.

Alan Stern

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Dmitry Torokhov]

On Mon, Nov 30, 2015 at 11:06:30AM -0500, Alan Stern wrote:
> On Mon, 30 Nov 2015, Vladis Dronov wrote:
> 
> > Hello, Alan, all.
> 
> Hello.
> 
> > > > Yes for the normal USB device. This case is a weird USB device (with no
> > > > endpoints) specially designed to be weird. My point here is that even a
> > > > weird device probing should not crash a kernel by a NULL-deref.
> > > 
> > > True.  However, a weird device that didn't have any endpoint 0 would
> > > not crash the kernel, because the kernel wouldn't be able to
> > > communicate with it at all.
> > 
> > I'm afraid, I do not get you here. A device in question _does_ crash the kernel
> > (or driver only) as this call trace shows (see attached for the full call trace),
> > and that's why the patch is proposed:
> > 
> > [  622.444650] BUG: unable to handle kernel NULL pointer dereference at 0000000000000002
> > [  622.445019] IP: [<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
> > [  622.445019] RIP: 0010:[<ffffffffa0395303>] aiptek_probe+0x463/0x658 [aiptek]
> > [  622.445019] RAX: 0000000000000000 RBX: ffff88000bd67800 RCX: ffff88000bcd0800
> >                         ^^^ rax is NULL, it is being dereferenced
> 
> This is the result of a bug in the aiptek driver.  See below.
> 
> > Kernel does not communicates with the device,
> 
> Yes it does.  Otherwise how would the kernel know that the aiptek
> driver was the one which should be probed for this device?
> 
> >  but the driver tries to access
> > a kernel structure, which was not allocated (thus, a null-deref):
> > 
> > endpoint = &intf->altsetting[0].endpoint[0].desc; // the structure endpoint[0]
> >                                              // was not kzalloc()ed, thus NULL
> > usb_rcvintpipe(aiptek->usbdev, endpoint->bEndpointAddress) // NULL deref
> 
> Right.  That's the bug; aiptek should not try to access a data 
> structure that was never allocated.
> 
> > > Drivers do not have to check whether endpoint 0 exists; it always does.  
> > > But they do have to check any other endpoint they use.
> > 
> > As the above example shows, endpoint 0 does not always exists. A specially
> > crafted weird USB device can advertise absence of endpoint 0 and the kernel
> > will accept this:
> 
> The example above has nothing to do with endpoint 0.  The entries in
> the altsetting[n].endpoint array are not stored in numerical order;  
> entry 0 in this array does not refer to endpoint 0.  (See the comment
> for the endpoint field in the definition of struct usb_host_interface
> in include/linux/usb.h.)  In fact, endpoint 0 is treated specially and
> isn't part of this array at all.
> 
> > [drivers/usb/core/config.c]
> > num_ep = num_ep_orig = alt->desc.bNumEndpoints;  // weird device can have 0 here
> > alt->desc.bNumEndpoints = 0;
> > if (num_ep > 0) {
> >         /* Can't allocate 0 bytes */
> >         len = sizeof(struct usb_host_endpoint) * num_ep;
> >         alt->endpoint = kzalloc(len, GFP_KERNEL);
> > 
> > As far as I understand, this is a question we discuss here: whose responsibility
> > is to handle situations of endpoint 0 absence in an altconfig?
> 
> num_ep counts the number of endpoints _other_ than endpoint 0.  
> There's no point counting endpoint 0 because it is always present.  
> Not to mention that it doesn't get stored in this array, since endpoint
> 0 has no descriptor.
> 
> > - if it is driver's job, a driver much check this, as in the patch I propose
> > 
> > - if it is a kernel job not to allow devices with no endpoint 0 in one the
> > configurations, the current USB device detection implementation (in
> > drivers/usb/core/config.c) should be modified, as currently it allows such.
> > 
> > I do not have much expertise in the Linux USB stack, so I'm asking you and
> > the community for an advise on this.
> 
> My advice is to fix aiptek's probe routine.  It should check that 
> intf->altsetting[0].desc.bNumEndpoints > 0 before trying to accessing 
> the endpoint array.
> 
> > > > btw, indeed, this is not the only driver with this problem, I've met 2 more.
> > > 
> > > In fact, there's no way to check whether endpoint 0 exists.  Where 
> > > would you look to find out?  There isn't any endpoint descriptor for it.
> > 
> > I believe, there is a configuration descriptor for that, probably, I'm wrong:
> > 
> > if (intf->altsetting[0].desc.bNumEndpoints < 1) { ...
> 
> That value is the number of endpoints _other_ than endpoint 0.  This is 
> documented in the USB-2.0 specification, Table 9-12.

How about we do something like below?

Thanks.

-- 
Dmitry

usb: interface: allow drivers declare number of endpoints they need

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

USB interface drivers need to check number of endpoints before trying to
access/use them. Quite a few drivers only use the default setting
(altsetting 0), so let's allow them to declare number of endpoints in
altsetting 0 they require to operate and have USB core check it for us
instead of having every driver implement check manually.

For compatibility, if driver does not specify number of endpoints (i.e.
number of endpoints is left at 0) we bypass the check in USB core and
expect the driver perform necessary checks on its own.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/usb/core/driver.c |    9 +++++++++
 include/linux/usb.h       |    7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 6b5063e..d9f680d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
 
 	dev_dbg(dev, "%s - got id\n", __func__);
 
+	if (driver->num_endpoints &&
+	    intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
+
+		dev_err(dev, "Not enough endpoints %d (want %d)\n",
+			intf->altsetting[0].desc.bNumEndpoints,
+			driver->num_endpoints);
+		return -EINVAL;
+	}
+
 	error = usb_autoresume_device(udev);
 	if (error)
 		return error;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447fe29..93f8dfc 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
  * @id_table: USB drivers use ID table to support hotplugging.
  *	Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
  *	or your driver's probe function will never get called.
+ * @num_endpoints: Number of endpoints that should be present in default
+ *	setting (altsetting 0) the driver needs to operate properly.
+ *	The probe will be aborted if actual number of endpoints is less
+ *	than what the driver specified here. 0 means no check should be
+ *	performed.
  * @dynids: used internally to hold the list of dynamically added device
  *	ids for this driver.
  * @drvwrap: Driver-model core structure wrapper.
@@ -1099,6 +1104,8 @@ struct usb_driver {
 
 	const struct usb_device_id *id_table;
 
+	unsigned int num_endpoints;
+
 	struct usb_dynids dynids;
 	struct usbdrv_wrap drvwrap;
 	unsigned int no_dynamic_id:1;

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Alan Stern]

On Mon, 30 Nov 2015, Dmitry Torokhov wrote:

> How about we do something like below?

It looks okay to me.  If you want to propose it on the linux-usb 
mailing list, you can add:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Dmitry Torokhov]

On Mon, Nov 30, 2015 at 03:38:20PM -0500, Alan Stern wrote:
> On Mon, 30 Nov 2015, Dmitry Torokhov wrote:
> 
> > How about we do something like below?
> 
> It looks okay to me.  If you want to propose it on the linux-usb 
> mailing list, you can add:
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

I will, thank you Alan.

-- 
Dmitry

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Vladis Dronov]

Hello, Alan,

Thank you for the explanations and I'm sorry for my terminology
misunderstanding, I got the point now, endpoint 0 != endpoint[0].

> My advice is to fix aiptek's probe routine. It should check that 
> intf->altsetting[0].desc.bNumEndpoints > 0 before trying to accessing 
> the endpoint array.

Thank you, yes, the patch proposed does something quite alike.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Alan Stern]

On Tue, 1 Dec 2015, Vladis Dronov wrote:

> Hello, Alan,
> 
> Thank you for the explanations and I'm sorry for my terminology
> misunderstanding, I got the point now, endpoint 0 != endpoint[0].

That's okay; it's an understandable mistake.

> > My advice is to fix aiptek's probe routine. It should check that 
> > intf->altsetting[0].desc.bNumEndpoints > 0 before trying to accessing 
> > the endpoint array.
> 
> Thank you, yes, the patch proposed does something quite alike.

You're welcome.  Glad I could help.

Alan Stern

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Dmitry Torokhov]

Hi Vladis,

On Wed, Nov 25, 2015 at 04:58:08PM +0100, Vladis Dronov wrote:
> The aiptek driver crashes in aiptek_probe() when a specially crafted usb device
> without endpoints is detected. This fix adds a check that the device has proper
> configuration expected by the driver. Also an error return value is changed to
> more matching one in one of the error paths.
> 
> Reported-by: Ralf Spenneberg <ralf@spenneberg.net>
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
>  drivers/input/tablet/aiptek.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
> index e7f966d..78c0732 100644
> --- a/drivers/input/tablet/aiptek.c
> +++ b/drivers/input/tablet/aiptek.c
> @@ -1819,6 +1819,15 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	input_set_abs_params(inputdev, ABS_TILT_Y, AIPTEK_TILT_MIN, AIPTEK_TILT_MAX, 0, 0);
>  	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) {
> +		dev_warn(&intf->dev,

This should be dev_err as we are aborting device initialization. I know
the driver user warn/info in similar places, but it is not right, we
might want to adjust it at some point.

> +			"interface has %d endpoints, but must have minimum 1\n",
> +			intf->altsetting[0].desc.bNumEndpoints);
> +		err = -ENODEV;

-EINVAL: the device configuration is invalid from the driver point of
view.

> +		goto fail3;
> +	}
>  	endpoint = &intf->altsetting[0].endpoint[0].desc;
>  
>  	/* Go set up our URB, which is called when the tablet receives
> @@ -1861,6 +1870,7 @@ aiptek_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	if (i == ARRAY_SIZE(speeds)) {
>  		dev_info(&intf->dev,
>  			 "Aiptek tried all speeds, no sane response\n");
> +		err = -ENODEV;

I believe it should be -EINVAL as well. I adjusted the above 3 items and
applied.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints

[Vladis Dronov]

Hello, Dmitry,

> > +	if (intf->altsetting[0].desc.bNumEndpoints < 1) {
> > +		dev_warn(&intf->dev,
>
> This should be dev_err as we are aborting device initialization. I know
> the driver user warn/info in similar places, but it is not right, we
> might want to adjust it at some point.

Yes, the driver using dev_warn() in all the similar error paths was a
reason for me to use dev_warn() here.

> > +		err = -ENODEV;
>
> I believe it should be -EINVAL as well. I adjusted the above 3 items
> and applied.

Indeed, this suits best. Thank you for handling this and the issue in
general.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer