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

From: Vladis Dronov <vdronov@redhat.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>, linux-input@vger.kernel.org
Subject: Re: [PATCH] Input: aiptek: fix crash on detecting device without endpoints
Date: Mon, 30 Nov 2015 07:36:47 -0500 (EST)	[thread overview]
Message-ID: <689469584.28015553.1448887007112.JavaMail.zimbra@redhat.com> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1511271650030.17118-100000@netrider.rowland.org>

[-- 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