Input: aiptek: fix crash on detecting device without endpoints
diff mbox

Message ID 1448467088-7703-1-git-send-email-vdronov@redhat.com
State Accepted
Headers show

Commit Message

Vladis Dronov Nov. 25, 2015, 3:58 p.m. UTC
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(+)

Comments

dmitry.torokhov@gmail.com Nov. 26, 2015, 4:36 p.m. UTC | #1
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
>
Alan Stern Nov. 26, 2015, 5:35 p.m. UTC | #2
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

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladis Dronov Nov. 27, 2015, 10:50 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Nov. 27, 2015, 9:54 p.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladis Dronov Nov. 30, 2015, 12:36 p.m. UTC | #5
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
[  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
Alan Stern Nov. 30, 2015, 4:06 p.m. UTC | #6
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

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladis Dronov Dec. 1, 2015, 8:09 a.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Dec. 1, 2015, 3:07 p.m. UTC | #8
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

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
dmitry.torokhov@gmail.com Dec. 1, 2015, 9:16 p.m. UTC | #9
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.
Vladis Dronov Dec. 2, 2015, 11:03 a.m. UTC | #10
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 	}