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
