Paths

Table of Contentst

bthidd: Consider usage ranges when dealing with array inputs.
ClosedPublic
Actions

Authored by rakuco on Apr 5 2015, 5:41 PM.

Details

Reviewers

emax
mav

Commits

rS281568: MFC r281116.
rS281566: MFC r281116.
rS281116: bthidd: Consider usage ranges when dealing with array inputs.

Summary

So far, we were always using HID_USAGE() to determine the Usage ID of a
certain HID report input item. This does not work as intended if a field
is an array and the allowed usages are specified with a usage range, as
HID_USAGE() will return 0. We need to use the field value as an index in
the usage range list in this case instead.

This makes the volume keys in a Microsoft Bluetooth Mobile Keyboard
5000 be properly recognized. The relevant part of the HID report looks
like this:

0xA1, 0x01,        // Collection (Application)
0x85, 0x07,        //   Report ID (7)
0x05, 0x0C,        //   Usage Page (Consumer)
0x19, 0x00,        //   Usage Minimum (Unassigned)
0x2A, 0xFF, 0x03,  //   Usage Maximum (0x03FF)
0x95, 0x01,        //   Report Count (1)
0x75, 0x10,        //   Report Size (16)
0x15, 0x00,        //   Logical Minimum (0)
0x27, 0xFF, 0x03, 0x00, 0x00,  //   Logical Maximum (1023)
0x81, 0x00,        //   Input (Data,Array,Abs,No Wrap,Linear,Preferred
                   //   State,No Null Position)

When a key such as "volume down" is pressed, the following data is
transferred through Interrupt In:

0x07 0xEA 0x00

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rakuco updated this revision to Diff 4680.Apr 5 2015, 5:41 PM

rakuco retitled this revision from to bthidd: Consider usage ranges when dealing with array inputs..

rakuco added reviewers: emax, mav.

rakuco updated this object.

Herald added a subscriber: imp. · View Herald TranscriptApr 5 2015, 5:41 PM

I'd appreciate some feedback here. Should I check for overflows in the code I've added? Does it make sense that calls to hid_get_item() return a single item for an array even if the report count is > 1? The keyboard handling code that manually works around it, but I wonder if this should be done in libusbhid (and in the analogous kernel code) instead.

please see minor inline comment. thank you for your work!

usr.sbin/bluetooth/bthidd/hid.c
181	can you please switch blocks within if() so it looks like if (( h & HIO_VARIABLE)) { usage = HID_USAGE(..); } else { new code; }. its easier to read this way. also usage_offset can be made local (it does not seems be used anywhere else). otherwise, looks fine.

Patch updated with the if check inverted. I thought all variables were supposed to be declared at the top, thankfully it doesn't seem to be the case :-)

Is the code enough as-is or should I add some overflow checks?

emax added inline comments.Apr 5 2015, 5:59 PM

usr.sbin/bluetooth/bthidd/hid.c
176	please add extra set of brackets around h & HIO_VARIABLE, i.e. if ((h & HIO_VARIABLE)). this is to avoid possible warning. can you please explain which overflow are you worrying here about?

Additional ()'s added to the if check.

looks fine to me.

Weird cases that I have in mind:

h.logical_minimum being larger than val.
(h.usage_minimum + usage_offset) > h.usage_maximum.
An array being defined with a usage ID instead of a usage range (all items would have the same usage, which does not make much sense to me and I haven't seen any examples of this so far).

i don't think its worth worrying about. HID_USAGE is basically

#define HID_USAGE(u) ((u) & 0xffff)

so, if HID descriptor and/or value is wrong (i.e. out of range), nothing terrible would happen (imo).

emax accepted this revision.Apr 5 2015, 6:34 PM

emax edited edge metadata.

This revision is now accepted and ready to land.Apr 5 2015, 6:34 PM

Closed by commit rS281116 (authored by @rakuco).

Revision Contents
Changeset List

Path

Size

usr.sbin/

bluetooth/

bthidd/

hid.c

14 lines

Diff 4682

View Options

usr.sbin/bluetooth/bthidd/hid.c

bthidd: Consider usage ranges when dealing with array inputs.ClosedPublicActions