| Message ID | 1448467088-7703-1-git-send-email-vdronov@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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
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
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
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
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
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
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
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.
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
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; }
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(+)