Bug 80091

Summary: NULL pointer dereference in hid_rmi
Product: Drivers Reporter: Dmitry (nrndda)
Component: Input DevicesAssignee: drivers_input-devices
Status: RESOLVED CODE_FIX    
Severity: normal CC: aduggan, andrey_utkin
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 3.16.0-rc4 Subsystem:
Regression: No Bisected commit-id:
Attachments: dmesg
.config
printk()s for Bug 80091 - NULL pointer dereference in hid_rmi
dmesg after modprobing hid-rmi with printks
Enabled kernel debug options and cmdline.
null pinter dereference fix
dmesg output after patch
lsusb -vvvv from ubuntu
syslog from ubuntu
current dmesg with working touchpad (until pressing keys) and don't working keyboard
this patch works
patch to only bind to mouse interfaces

Description Dmitry 2014-07-12 11:08:01 UTC
Modprobing hid_rmi for this device:
Synaptics T Pad V 01.30 UNKNOWN as /devices/pci0000:00/0000:00:14.0/usb1/1-4/1-4:1.0/0003:06CB:2819.0003/input/input8
leads to null pointer dereference.

Platform: Dell Venue 11 Pro with keyboard dock.

I failed in debugging with either gdb or dynamic_debug. Probably I'll manage to use printks if somebody tell me where to put them;)
Comment 1 Dmitry 2014-07-12 11:08:26 UTC
Created attachment 142761 [details]
dmesg
Comment 2 Dmitry 2014-07-12 11:14:55 UTC
For me it looks close to bugs 56251 and 77341.
Comment 3 Dmitry 2014-07-12 11:15:53 UTC
Created attachment 142771 [details]
.config
Comment 4 Andrey Utkin 2014-07-13 09:43:21 UTC
Created attachment 142841 [details]
printk()s for Bug 80091 - NULL pointer dereference in hid_rmi

Still there? Could you please try this?
I suppose either hdev->report_enum[HID_INPUT_REPORT].report_id_hash or hdev->report_enum[HID_OUTPUT_REPORT].report_id_hash is null.
Comment 5 Dmitry 2014-07-13 18:50:42 UTC
Created attachment 142851 [details]
dmesg after modprobing hid-rmi with printks

This is everything what I got. Maybe I haven't turned something on?
Comment 6 Dmitry 2014-07-13 18:51:41 UTC
Created attachment 142861 [details]
Enabled kernel debug options and cmdline.
Comment 7 Andrey Utkin 2014-07-13 19:07:24 UTC
Well, i think i have added wrong checks, but now we definitely know that the problem is the line

data->input_report_size = (hdev->report_enum[HID_INPUT_REPORT]
        .report_id_hash[RMI_ATTN_REPORT_ID]->size >> 3)
        + 1 /* report id */;

And the NULL expression is likely hdev->report_enum[HID_INPUT_REPORT].report_id_hash[RMI_ATTN_REPORT_ID].
Comment 8 Andrey Utkin 2014-07-13 19:24:00 UTC
Looking at drivers/hid/hid-magicmouse.c, i see that we should do something like 

report = hid_register_report(hdev, HID_INPUT_REPORT, RMI_ATTN_REPORT_ID); report->size = XXX;

before we use hdev->report_enum[HID_INPUT_REPORT].report_id_hash[RMI_ATTN_REPORT_ID].
I have no idea what should be this "size", but you can try to set to some "reasonably high" number.

Another way should be to avoid usage of those expressions in the calculation of allocation sizes. But i have no idea how to calculate them otherwise.
Comment 9 Dmitry 2014-07-14 21:38:31 UTC
OK, I tried with patch below and it fixed null pointer dereference. But module doesn't work.
Comment 10 Dmitry 2014-07-14 21:39:02 UTC
Created attachment 143091 [details]
null pinter dereference fix
Comment 11 Dmitry 2014-07-14 21:39:27 UTC
Created attachment 143101 [details]
dmesg output after patch
Comment 12 Dmitry 2014-07-14 21:44:51 UTC
There are some mistakes in printks but values are right.
So, it isn't enough for make module work.
Comment 13 Andrey Utkin 2014-07-14 21:46:32 UTC
As i said, try to set report->size after hid_register_report().
Also i _guess_ setting hdev->report_enum[...].report_id_hash[...] with result of hid_register_report() is unnecessary.
Comment 14 Andrey Utkin 2014-07-14 21:56:08 UTC
I have mailed to authors of the driver, also you should mail to linux-usb maillist.
Comment 15 Dmitry 2014-07-14 22:28:25 UTC
This don't help no matter what size I choose:

Jul 15 02:15:07 venue11pro kernel: hid-rmi 0003:06CB:2819.0004: unable to set rmi mode to 1 (-32)
Jul 15 02:15:07 venue11pro kernel: hid-rmi 0003:06CB:2819.0004: failed to set rmi mode

Jul 15 02:15:12 venue11pro kernel: hid-rmi 0003:06CB:2819.0005: failed to write hid report (-110)
Jul 15 02:15:12 venue11pro kernel: hid-rmi 0003:06CB:2819.0005: rmi_set_page: set page failed: -110.
Jul 15 02:15:12 venue11pro kernel: hid-rmi 0003:06CB:2819.0005: failed to set page select to 0.

Tried with (64>>3)+1=9 and (128>>3)+1=17 bits.
Comment 16 Andrew Duggan 2014-07-15 01:19:58 UTC
There does seem to be something wrong here.

This touchpad isn't actually supported by hid-rmi. This touchpad complies with the Microsoft Precision Touchpad spec which reports data a little differently, but it is supported by the hid-multitouch driver. The hid-core driver should recognize this when it parses the HID descriptor and bind the hid-multitouch driver to it. This makes me think that hid-core failed to properly parse the HID descriptors. Incidentally, because hid-core isn't recognizing the touchpad correctly hid-rmi is getting loaded by default.

Even though hid-rmi doesn't support this type of touchpad it shouldn't be failing like this. There should still be an attention report ID so setting the input report size shouldn't result in a null pointer derefernce. I guess I will have to add some additional error checking here. Also the error codes reported by rmi_set_mode (-32) and rmi_write_report (-110) may be coming from the USB layer so there maybe something wrong with how the device is communicating.

Did you try docking and undocking a few times?
Did the touchpad work with previous versions of the kernel?
Did the touchpad work under Windows? Just to rule out that the hardware isn't completely broken on this unit.

I will see if I can find a system and try to reproduce this issue.
Comment 17 Dmitry 2014-07-15 12:55:27 UTC
This dock contains not only touchpad but also a keyboard. Exactly the latter don't work. I've tested with ubuntu with kernel 3.13 and keyboard works.
That is xinput from 3.16:
⎡ Virtual core pointer                          id=2    [master pointer  (3)]
⎜   ↳ Virtual core XTEST pointer                id=4    [slave  pointer  (2)]
⎜   ↳ Synaptics T Pad V 01.30 UNKNOWN           id=7    [slave  pointer  (2)]
⎜   ↳ SYNA7500:00 06CB:3AF0 Pen                 id=8    [slave  pointer  (2)]
⎜   ↳ SYNA7500:00 06CB:3AF0                     id=9    [slave  pointer  (2)]
⎣ Virtual core keyboard                         id=3    [master keyboard (2)]
    ↳ Virtual core XTEST keyboard               id=5    [slave  keyboard (3)]
    ↳ Video Bus                                 id=6    [slave  keyboard (3)]
    ↳ gpio-keys                                 id=10   [slave  keyboard (3)]
    ↳ gpio-keys                                 id=11   [slave  keyboard (3)]
    ↳ Dell WMI hotkeys                          id=12   [slave  keyboard (3)]

And from ubuntu:
⎡ Virtual core pointer                          id=2    [master pointer  (3)]
⎜   ↳ Virtual core XTEST pointer                id=4    [slave  pointer  (2)]
⎜   ↳ SYNA7500:00 06CB:3AF0 Pen                 id=10   [slave  pointer  (2)]
⎜   ↳ SYNA7500:00 06CB:3AF0                     id=11   [slave  pointer  (2)]
⎜   ↳ Synaptics T Pad V 01.30                   id=7    [slave  pointer  (2)]
⎣ Virtual core keyboard                         id=3    [master keyboard (2)]
    ↳ Virtual core XTEST keyboard               id=5    [slave  keyboard (3)]
    ↳ Video Bus                                 id=6    [slave  keyboard (3)]
    ↳ Synaptics T Pad V 01.30                   id=8    [slave  keyboard (3)]
    ↳ Synaptics T Pad V 01.30                   id=9    [slave  keyboard (3)]
    ↳ Dell WMI hotkeys                          id=12   [slave  keyboard (3)]

In both outputs touchpad does work. hid-rmi blocked in 3.16 and in 3.13 kernel it desn't exist (not yet backported).
Suddenly I found that pressing any key on keyboard disables touchpad completely.
In UEFI both keyboard and touchpad work.
Comment 18 Dmitry 2014-07-15 12:56:42 UTC
Created attachment 143121 [details]
lsusb -vvvv from ubuntu
Comment 19 Dmitry 2014-07-15 12:57:04 UTC
Created attachment 143131 [details]
syslog from ubuntu
Comment 20 Dmitry 2014-07-15 13:05:29 UTC
Created attachment 143141 [details]
current dmesg with working touchpad (until pressing keys) and don't working keyboard
Comment 21 Andrew Duggan 2014-07-16 00:34:58 UTC
Ok, looks like this is a composite USB device with multiple interfaces for the HID keyboard and the HID touchpad. The strange thing is that they are using our Vendor ID to identify this composite device. Because the USB device has the Synaptics Vendor ID hid-core is trying to bind hid-rmi to the keyboard and whatever the third interface is. But, since those devices don't have the correct report ids hid-rmi ends up dereferencing a null pointer.

I can add checks for null pointers, but the main issue is that hid-rmi is incorrectly being bound to a device it should not be. These devices should probably be using hid-generic instead. It looks like we will have to reexamining the following lines since we can no longer solely rely on our Vendor ID distinguishing our devices.

hid-core.c:785
if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
	(hid->group == HID_GROUP_GENERIC))
		/* hid-rmi should take care of them, not hid-generic */
                hid->group = HID_GROUP_RMI;

I'm not sure if typing on the keyboard resulting the in the touchpad being disabled is related or not though.
Comment 22 Dmitry 2014-07-16 05:02:40 UTC
Created attachment 143201 [details]
this patch works

Yep, you were right! This patch works for me. Both device at same time. Of course it's a hack, but now it's clear where the problem is. Thanks for help!
Comment 23 Andrew Duggan 2014-07-17 00:44:05 UTC
Created attachment 143301 [details]
patch to only bind to mouse interfaces

I came up with a patch should be more generic. If you get a chance can you revert your patch and try it to make sure it works. If so I will submit it upstream and see what they think of it. I wasn't able to get my hands on hardware after all.
Comment 24 Dmitry 2014-07-17 16:32:33 UTC
I confirm, this patch works too. Of course after reverting my old patch.
Comment 25 Andrew Duggan 2014-07-17 23:23:01 UTC
Thanks for testing!