Fork us on GitHub Follow us on Facebook Follow us on Twitter

Opened 8 years ago

Closed 8 years ago

#349 closed defect (fixed)

usbhid hit a mallocassertion when mouse is moved for the first time

Reported by: Jakub Jermář Owned by: Vojtech Horky
Priority: major Milestone: 0.5.0
Component: helenos/drv/usbhid Version: mainline
Keywords: usb, hid, malloc Cc: vojtech.horky@…
Blocker for: Depends on:
See also:

Description

When testing the USB framework (mainline,1035) on my desktop machine, I hit the following malloc assertion in the usbhid task when having moved the mouse for the first time after it attached:

assert_abort+93
malloc_area+1021
malloc_internal+198
malloc+32
usb_hid_report_path_append_item+23
usb_hid_report_get_sibling+124
usb_mouse_polling_callback+1000
usb_hid_polling_callback+158

Attachments (2)

descriptor.JPG (1.1 MB) - added by Jakub Jermář 8 years ago.
Screenshot depicting the HID device descriptor in a raw format.
lsusb (4.1 KB) - added by Jakub Jermář 8 years ago.
Linux lsusb -v of the mouse device.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 years ago by Vojtech Horky

Cc: vojtech.horky@… added

comment:2 Changed 8 years ago by Jan Vesely

vojta, would it be possible to use the report descriptor with virtual usb hc and replicate this without that special kind of mouse?

comment:3 Changed 8 years ago by Vojtech Horky

That shall be possible. All we need is the report descriptor in some raw format.

Jakub: can you post it here? It shall be possible via usbinfo -r <device address> from HelenOS.

It shall be possible to get it somehow directly from Linux, but lsusb -v is trying to interpret it and I do not know of any way to force some raw dump.

Changed 8 years ago by Jakub Jermář

Attachment: descriptor.JPG added

Screenshot depicting the HID device descriptor in a raw format.

comment:4 Changed 8 years ago by Jakub Jermář

Ok, here is an OCR'ed and hand-edited text version of the descriptors:

HID report descriptor for interface 0
05 01 09 06 A1 01 05 07 19 E0 29 E7 15 00 25 01 75 01 95 08
81 02 81 03 95 05 05 08 19 01 29 05 91 02 95 01 75 03 91 01
95 06 75 08 15 00 26 A4 00 05 07 19 00 2A A4 00 81 00 C0

HID report descriptor for interface 1
05 01 09 02 A1 01 85 02 09 01 A1 00 05 09 19 01 29 10 15 00
25 01 95 10 75 01 81 02 05 01 16 01 F8 26 FF 07 75 0C 95 02
09 30 09 31 81 06 15 81 25 7F 75 08 95 01 09 38 81 06 05 0C
0A 38 02 95 01 81 06 C0 C0 05 0C 09 01 A1 01 85 03 75 10 95
02 15 01 26 8C 02 19 01 2A 8C 02 81 00 C0 05 01 09 80 A1 01
85 04 75 02 95 01 15 01 25 03 09 82 09 81 09 83 81 60 75 06
81 03 C0 06 BC FF 09 88 A1 01 85 08 19 01 29 FF 15 01 26 FF
00 75 08 95 01 81 00 C0

HID report descriptor for interface 2
06 00 FF 09 01 A1 01 85 10 75 08 95 06 15 00 26 FF 00 09 01
81 00 09 01 91 00 C0 06 00 FF 09 02 A1 01 85 11 75 08 95 13
15 00 26 FF 00 09 02 81 00 09 02 91 00 C0 06 00 FF 09 04 A1
01 85 20 75 08 95 0E 15 00 26 FF 00 09 41 81 00 09 41 91 00
85 21 95 1F 15 00 26 FF 00 09 42 81 00 09 42 91 00 C0
Last edited 8 years ago by Jakub Jermář (previous) (diff)

Changed 8 years ago by Jakub Jermář

Attachment: lsusb added

Linux lsusb -v of the mouse device.

comment:5 Changed 8 years ago by Vojtech Horky

Owner: set to Vojtech Horky
Status: newaccepted

Just a progress info if anyone is interested.

I created a virtual USB HID simulating this device. It is currently available in my USB branch: lp:~vojtech-horky/helenos/usb. To simulate the device, enable Virtual USB host controller in the configuration menu and from bdsh launch vuh lw1 (for Logitech Wireless, interface one). The usbhid driver will crash with similar stack trace as mentioned in the bug description.

It looks that the driver corrupts the memory allocator structures by writing past the allocated area. If you add the red_zone field to heap_block_foot_t in uspace/lib/c/generic/malloc.c:

/** Footer of a heap block
 *
 */
typedef struct {
        uint8_t red_zone[41];

        /* Size of the block (including header and footer) */
        size_t size;
        
        /* A magic value to detect overwrite of heap footer */
        uint32_t magic;
} heap_block_foot_t;

it will not crash. Descreasing the red zone length to 40 leads to assertion failure in the allocator.

Will report further progress.

comment:6 Changed 8 years ago by Vojtech Horky

I think I found the cause of the trouble.

In the HID driver (mouse/mousedev.c) there is a hardcoded limit for number of mouse buttons

static const size_t USB_MOUSE_BUTTON_COUNT = 3;

and I guess that the mouse in question has more of them (or the descriptor is structured differently).

Thus accessing the buttons field usb_mouse_process_report()

mouse_dev->buttons[field->usage - field->usage_minimum] = field->value;

can cause buffer overrun, corrupting heap allocator internal structures.

The fix is to scan the report descriptor first to determine number of buttons and allocate the buttons array accordingly. I will fix that but it may take me some time (I do not know much about the libusbhid).

Jakub, can you please verify that setting the constant to something bigger (20 worked in the simulation) works on real hardware? Thanks.

comment:7 Changed 8 years ago by Jakub Jermář

Changing USB_MOUSE_BUTTON_COUNT to 20 makes the crash non-reproducible. The mouse seems to be extremely sensitive to moves then, but other than that, it seems to work properly.

comment:8 Changed 8 years ago by Vojtech Horky

Hopefully fixed in lp:~vojtech-horky/helenos/usb:1073.

Jakub, can you, please, test it? Thanks.

comment:9 Changed 8 years ago by Jakub Jermář

After locally merging your entire branch the crash becomes non-reproducible and the mouse is functional. Thanks for fixing this!

comment:10 Changed 8 years ago by Vojtech Horky

Resolution: fixed
Status: acceptedclosed

Merged into mainline,1244.

Note: See TracTickets for help on using tickets.