Linux USB HID should ignore values outside Logical Minimum/Maximum range

DISCUSSION Denilson Figueiredo de Sá 5 participants 14 messages Oct 22, 2011 – Nov 16, 2011 HID

Share & tools

Denilson Figueiredo de Sá

author

2011-10-22 11:42 UTC #0 lore raw

Short description:

An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing values outside that range.

Linux kernel should ignore values outside that range, as they are not meaningful.

Just for comparison, Windows ignores such values. (and I hate this kind of comparison)

Long description:

I'm building a homebrew USB Absolute Pointing Device using an AVR ATmega8 and the V-USB firmware. This device will work almost like a mouse, with one (obvious) difference: while a mouse sends "relative" movements (move the pointer x,y units, relative to its current position), an absolute pointing device sends "absolute" positions (move the mouse exactly to this x,y location).

This device is using two 16-bit fields to report the X and Y axes, and limit the meaningful values to those within the [0..32767] range. This is the relevant part of the USB HID Descriptor:

	0x09, 0x30,        //     USAGE (X)
	0x09, 0x31,        //     USAGE (Y)
	0x15, 0x00,        //     LOGICAL_MINIMUM (0)
	0x26, 0xff, 0x7f,  //     LOGICAL_MAXIMUM (32767)
	0x75, 0x10,        //     REPORT_SIZE (16)
	0x95, 0x02,        //     REPORT_COUNT (2)
	0x81, 0x42,        //     INPUT (Data,Var,Abs,Null)

However, Linux kernel is not behaving as expected when receiving out-of-range values. Instead of ignoring such coordinates, it moves the pointer to the bottom-right corner of the screen.

Each HID report for this device has 1 byte for the Report ID (with value 02), 2 bytes for the X position, 2 bytes for the Y position, and finally 1 byte for the button presses. They shows up as "02XXXXYY YYbb" in usbmon.

Look at this usbmon output:

ffff88000c9a0600 1465973871 C Ii:5:093:1 0:8 6 = 02ff0fff 0f00 ffff88000c9a0600 1465973936 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1467317888 C Ii:5:093:1 0:8 6 = 02ff1fff 1f00 ffff88000c9a0600 1467317950 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1467725888 C Ii:5:093:1 0:8 6 = 02ff2fff 2f00 ffff88000c9a0600 1467725931 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1468661905 C Ii:5:093:1 0:8 6 = 02ff3fff 3f00 ffff88000c9a0600 1468661961 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1469429914 C Ii:5:093:1 0:8 6 = 02ff4fff 4f00 ffff88000c9a0600 1469429969 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1470133907 C Ii:5:093:1 0:8 6 = 02ff5fff 5f00 ffff88000c9a0600 1470133948 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1470837931 C Ii:5:093:1 0:8 6 = 02ff6fff 6f00 ffff88000c9a0600 1470837986 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1471445923 C Ii:5:093:1 0:8 6 = 02ff7fff 7f00 ffff88000c9a0600 1471445981 S Ii:5:093:1 -115:8 6 <

Here, the pointer is moving from 0x0fff, 0x0fff (near the top-left of the screen) up to the maximum possible value: 0x7fff, 0x7fff (the bottom-right corner). When receiving these reports, the pointer was correctly moving diagonally across my screen.

Up until here, no problem yet. The problem starts now:

ffff88000c9a0600 1472261948 C Ii:5:093:1 0:8 6 = 02ff8fff 8f00 ffff88000c9a0600 1472262007 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1472965956 C Ii:5:093:1 0:8 6 = 02ff9fff 9f00 ffff88000c9a0600 1472966010 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1473621964 C Ii:5:093:1 0:8 6 = 02ffafff af00 ffff88000c9a0600 1473622019 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1474285972 C Ii:5:093:1 0:8 6 = 02ffbfff bf00 ffff88000c9a0600 1474286027 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1474885980 C Ii:5:093:1 0:8 6 = 02ffcfff cf00 ffff88000c9a0600 1474886034 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1475517975 C Ii:5:093:1 0:8 6 = 02ffdfff df00 ffff88000c9a0600 1475518040 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1476333997 C Ii:5:093:1 0:8 6 = 02ffefff ef00 ffff88000c9a0600 1476334055 S Ii:5:093:1 -115:8 6 < ffff88000c9a0600 1477366011 C Ii:5:093:1 0:8 6 = 02ffffff ff00 ffff88000c9a0600 1477366069 S Ii:5:093:1 -115:8 6 <

These reports send values outside the range defined in the HID Descriptor. The values here go from 0x8fff up to 0xffff. All of these values are invalid and should have been ignored by the Linux kernel. Instead, what happened was that the kernel insisted into moving the pointer to the bottom-right corner of the screen.

Just to be sure about what I'm saying, I used my touchpad to move the pointer away from the corner before each one of those reports, and upon receiving them, the pointer went back to the corner.

It is worth noting that changing this:

	0x81, 0x42,        //     INPUT (Data,Var,Abs,Null)

to this:

	0x81, 0x02,        //     INPUT (Data,Var,Abs)

makes no difference at all.

I've also tried this device on a Windows XP system, and it works as expected there (out-of-range values are ignored and don't move the pointer).

It's also important to remember that an out-of-range value should be ignored, but not the full report. For instance, the device could have sent "02 ffff ffff 01", meaning an invalid X,Y position (which should be ignored), but a valid button press of the first button (which should be processed). In this case, the kernel should send a button_press event, but not a motion event.

It may even happen to send an out-of-range value for one axis, but a valid value for another axis. The code should be prepared for that (ignore one, but keep the other).

Found this bug in Linux kernel 2.6.38

If someone wants to build the hardware to reproduce this bug, go to the revision 662baa542e07 of this project:
https://bitbucket.org/denilsonsa/atmega8-magnetometer-usb-mouse/
It will probably need a few tweaks, though. Basically, after removing the non-relevant portions of the code (sensor, menu, keyemu), it should work using a fairly minimalistic ATmega8 circuit.

Signature · 2 lines

Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Chris Friesen

2011-10-24 16:24 UTC #1 lore raw

Quoted 3 lines · Denilson Figueiredo de Sá

It may even happen to send an out-of-range value for one axis, but a valid value for another axis. The code should be prepared for that (ignore one, but keep the other).

In this case what should be used for the "invalid" axis value? The previous value?

Chris

Signature · 9 lines

Chris Friesen
Software Developer
GENBAND
chris.friesen-b7o/[email protected]
www.genband.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Denilson Figueiredo de Sá

author

2011-10-24 16:39 UTC #2 lore raw

Quoted 8 lines · Chris Friesen

Quoted 3 lines · Denilson Figueiredo de Sá
It may even happen to send an out-of-range value for one axis, but a valid value for another axis. The code should be prepared for that (ignore one, but keep the other).
In this case what should be used for the "invalid" axis value? The previous value?

If that's an absolute pointing device, can we supply only one axis? I mean, can we do this: "move the pointer to this X position, but leave the Y position where it currently is". (note that the pointer might have moved since last input from this device, due to other devices also being present)

If that's not possible, then I'm not sure about what should be the correct behavior. Should we ignore both axes if one of them is invalid? Should we use the last good value from the other axis? (note that relative pointing devices like proper mice shouldn't have this issue, as we can just use zero for invalid values, which means "no movement")

I haven't tested that on Windows (yet), so I don't know how it behaves. If I test it, I'll post my results here.

I know, this is a corner case difficult to decide about.

By the way, my report was about absolute pointing devices, but I haven't tested how Linux USB HID behaves for other kinds of devices (keyboards, gamepads, anything else).

Signature · 6 lines

Denilson Figueiredo de Sá
Rio de Janeiro - Brasil
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov

2011-10-24 20:35 UTC #3 lore raw

Quoted 7 lines · Denilson Figueiredo de Sá

Quoted 6 lines · Chris Friesen
Quoted 3 lines · Denilson Figueiredo de Sá
It may even happen to send an out-of-range value for one axis, but a valid value for another axis. The code should be prepared for that (ignore one, but keep the other).

Quoted 6 lines

Quoted 2 lines
In this case what should be used for the "invalid" axis value? The previous value?
If that's an absolute pointing device, can we supply only one axis? I mean, can we do this: "move the pointer to this X position, but leave the Y position where it currently is". (note that the pointer might

Yes, input protocol is stateful and it will actually suppress duplicate events itself. I.e. if device moves from (10, 20) to (40, 20) usespace will see:

EV_ABS/ABS_X/10
EV_ABS/ABS_Y/20
EV_SYN/SYN_REPORT
EV_ABS/ABS_X/40
EV_SYN/SYN_REPORT

even if driver emits 2nd EV_ABS/ABS_Y/20 event.

Quoted 2 lines

have moved since last input from this device, due to other devices also being present)

The other devices have no effect on this device state.

Thanks.

Signature · 1 line

Dmitry

Denilson Figueiredo de Sá

author

2011-10-25 04:03 UTC #4 lore raw

In reply to Denilson Figueiredo de Sá

Quoted 7 lines · Denilson Figueiredo de Sá

Quoted 6 lines · Chris Friesen
Quoted 3 lines · Denilson Figueiredo de Sá
It may even happen to send an out-of-range value for one axis, but a valid value for another axis. The code should be prepared for that (ignore one, but keep the other).

Quoted 5 lines

Quoted 2 lines
In this case what should be used for the "invalid" axis value? The previous value?
I haven't tested that on Windows (yet), so I don't know how it behaves. If I test it, I'll post my results here.

I've modified the firmware of my device to do some more tests. I've modified it to send invalid values in X axis, but keep Y axis with valid values.

In Linux, the pointer moved to the right-most position of the screen (which is consistent with the behavior I previously described), not ignoring the out-of-range value.

In Windows, the pointer moved to the left-most position of the screen. It means Windows can't send a movement of only one axis, and fills the other axis with zero when receiving an invalid value. This behavior doesn't seem very helpful.

If anyone cares, this was the firmware modification for this test:
https://bitbucket.org/denilsonsa/atmega8-magnetometer-usb-mouse/changeset/31027323fa0b

Signature · 6 lines

Denilson Figueiredo de Sá
Rio de Janeiro - Brasil
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Fritz

2011-10-24 17:02 UTC #5 lore raw

In reply to Denilson Figueiredo de Sá

Hi Denilson,

please see my comment below.

Quoted 8 lines · Denilson Figueiredo de Sá

Short description:
An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing values outside that range.
Linux kernel should ignore values outside that range, as they are not meaningful.

To answer with a quote from the comment above input.h:"struct input_absinfo":

*
* Note that input core does not clamp reported values to the
* [minimum, maximum] limits, such task is left to userspace. *

to do so, use this from input.h:

#define EVIOCGABS(abs) _IOR('E', 0x40 + (abs), struct input_absinfo) /* get abs value/limits */
#define EVIOCSABS(abs) _IOW('E', 0xc0 + (abs), struct input_absinfo) /* set abs value/limits */

Quoted 1 line

<SNIP>

Thanks,
-- chf

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Denilson Figueiredo de Sá

author

2011-10-24 17:04 UTC #6 lore raw

Quoted 3 lines · Christoph Fritz

* Note that input core does not clamp reported values to the
* [minimum, maximum] limits, such task is left to userspace.

That means this is a bug in X.org, instead?

Signature · 2 lines

Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Dmitry Torokhov

2011-10-24 20:32 UTC #7 lore raw

In reply to Christoph Fritz

Hi Christoph,

Quoted 22 lines · Christoph Fritz

Hi Denilson,
please see my comment below.
Quoted 8 lines · Denilson Figueiredo de Sá
Short description:
An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing values outside that range.
Linux kernel should ignore values outside that range, as they are not meaningful.
To answer with a quote from the comment above input.h:"struct input_absinfo":
*
* Note that input core does not clamp reported values to the
* [minimum, maximum] limits, such task is left to userspace. *

Quoted 4 lines

to do so, use this from input.h:
#define EVIOCGABS(abs) _IOR('E', 0x40 + (abs), struct input_absinfo) /* get abs value/limits */
#define EVIOCSABS(abs) _IOW('E', 0xc0 + (abs), struct input_absinfo) /* set abs value/limits */

While input core does not clam or validate the values reported by the driver it might make sense to do so in hid-input; I am not sure how often these limits programmed intorrectly. Jiri i sprobably the best person to answer this question.

Thanks.

Signature · 1 line

Dmitry

Jiri Kosina

2011-10-28 16:23 UTC #8 lore raw

Quoted 4 lines · Dmitry Torokhov

While input core does not clam or validate the values reported by the driver it might make sense to do so in hid-input; I am not sure how often these limits programmed intorrectly. Jiri i sprobably the best person to answer this question.

I haven't met such a device myself yet.

On the other hand I feel like just dropping the report on the floor should be the proper thing to do.

Christoph, how about the (untested at my side yet) patch below? It'd be nice if you could test with the device you are seeing the problem with and let me know.

It's definitely not something I'll be pushing for 3.1 though; I'd first like to verify the batch of the device records I have (or I have received) to see whehter we can't introduce regressions this way.

From: Jiri Kosina <[email protected]>
Subject: [PATCH] HID: ignore absolute values which don't fit between logical min and max

Linux should ignore values outside logical min/max range, as they are not meaningful. This is what at least some of other OSes do, and it also makes sense (currently the value gets misinterpreted larger up the stack).

Reported-by Denilson Figueiredo de Sá <[email protected]>

Signed-off-by Jiri Kosina <[email protected]>

---
 drivers/hid/hid-input.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/hid/hid-input.c +7 -0

index f333139..ca923de 100644
@@ -822,6 +822,13 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	/* Ignore absolute values that are out of bounds */
+	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
+					value > field->logical_maximum))) {
+		dbg_hid("Ignoring out-of-range value %x\n", value);
+		return;
+	}
+
 	/* report the usage code as scancode if the key status has changed */
 	if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
 		input_event(input, EV_MSC, MSC_SCAN, usage->hid);

Signature · 6 lines

Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Fritz

2011-10-28 19:19 UTC #9 lore raw

Quoted 9 lines · Jiri Kosina

Quoted 4 lines · Dmitry Torokhov
While input core does not clam or validate the values reported by the driver it might make sense to do so in hid-input; I am not sure how often these limits programmed intorrectly. Jiri i sprobably the best person to answer this question.
I haven't met such a device myself yet.

Quoted 7 lines

On the other hand I feel like just dropping the report on the floor should be the proper thing to do.
Christoph, how about the (untested at my side yet) patch below? It'd be nice if you could test with the device you are seeing the problem with and let me know.

I don't have this device and I'm not the person who started this thread. It's Denilson Figueiredo de Sá <[email protected]> , just see above.

Denilson, please comment.

Thanks,
-- Christoph

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Denilson Figueiredo de Sá

author

2011-10-28 20:40 UTC #10 lore raw

In reply to Jiri Kosina

Quoted 2 lines · Jiri Kosina

On the other hand I feel like just dropping the report on the floor should be the proper thing to do.

I'd say not the entire report, but just the invalid fields.

The HID specification (HID1_11.pdf) talks about "Null Values" at the section 5.10 (page 20).

[quote]
HID class devices support the ability to ignore selected fields in a report at run-
time. This is accomplished by declaring bit field in a report that is capable of
containing a range of values larger than those actually generated by the control. If
the host or the device receives an out-of-range value then the current value for the
respective control will not be modified.
[/quote]

(sidenote: Why didn't I mention this spec earlier? Because I knew I have read that information somewhere, but didn't find it again until today.)

Quoted 3 lines

Christoph, how about the (untested at my side yet) patch below? It'd be nice if you could test with the device you are seeing the problem with and let me know.

I can't test it *right now*, but I can probably test it in a week or two.

drivers/hid/hid-input.c void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct · line 827

Quoted 3 lines

drivers/hid/hid-input.c +3 -0

+	/* Ignore absolute values that are out of bounds */
+	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
+					value > field->logical_maximum))) {

Given what the HID spec says (as I quoted above), I'd change this condition to just:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (value < field->logical_minimum ||
+					value > field->logical_maximum) {

On the other hand, HID spec also defines the "Null" bit of Input fields at page 31. This Null bit seems redundant for me, as (in my understanding) any value outside the range is automatically a null value. We wouldn't need a Null bit for that.

Anyway, another solution is to change that condition to something like this:

Review

Quoted 3 lines

(quoted diff) +3 -0

+	if ( THIS_INPUT_FIELD_HAS_NULL_BIT_SET &&
+                      (value < field->logical_minimum ||
+					value > field->logical_maximum)) {

What do you think?

Signature · 2 lines

Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Jiri Kosina

2011-10-31 15:24 UTC #11 lore raw

Quoted 4 lines · Denilson Figueiredo de Sá

Quoted 2 lines
On the other hand I feel like just dropping the report on the floor should be the proper thing to do.
I'd say not the entire report, but just the invalid fields.

Right, which is what my patch actually does, just my fingers on keyboard were faster than my brain while writing that.

Quoted 12 lines

The HID specification (HID1_11.pdf) talks about "Null Values" at the section 5.10 (page 20).
[quote]
HID class devices support the ability to ignore selected fields in a report at run- time. This is accomplished by declaring bit field in a report that is capable of containing a range of values larger than those actually generated by the control. If the host or the device receives an out-of-range value then the current value for the respective control will not be modified.
[/quote]

Quoted 2 lines

(sidenote: Why didn't I mention this spec earlier? Because I knew I have read that information somewhere, but didn't find it again until today.)

I personally find the wording of the spec here a bit unfortunate (the 'declaring bit field in a report that is capable of containing a range of values largen than those actually generated by the control' seems to be a bit too foggy and vague).

Quoted 5 lines

Quoted 3 lines
Christoph, how about the (untested at my side yet) patch below? It'd be nice if you could test with the device you are seeing the problem with and let me know.
I can't test it *right now*, but I can probably test it in a week or two.

Thanks. No real rush here, as I will definitely not be queuing this for 3.2 anyway, 3.3 is the first release I'd consider this for.

Quoted 9 lines

drivers/hid/hid-input.c void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct · line 827
Quoted 3 lines
drivers/hid/hid-input.c +3 -0
+	/* Ignore absolute values that are out of bounds */
+	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
+					value > field->logical_maximum))) {
Given what the HID spec says (as I quoted above), I'd change this condition to just:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (value < field->logical_minimum ||
+					value > field->logical_maximum) {

After thinking about it a little bit more, I think I agree.

I will still try to evaluate this against a number of devices I have (or I have reports for), to see if we potentially really can't break something.

So please let me know the result of your testing.

Thanks,

Signature · 6 lines

Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Denilson Figueiredo de Sá

author

2011-11-02 23:39 UTC #12 lore raw

Quoted 4 lines · Jiri Kosina

I personally find the wording of the spec here a bit unfortunate (the 'declaring bit field in a report that is capable of containing a range of values largen than those actually generated by the control' seems to be a bit too foggy and vague).

I don't think it is vague.

"declaring bit field in a report" -> Input()
"that is capable of containing a range of values" -> Report_size() "larger than those actually generated by the control" -> The entire range repreented using Report_size bits is larger than the Logical minimum/maximum,

For me, that paragraph is not foggy. Maybe other pieces of the spec are, but that one seemed clear for me.

Quoted 4 lines

Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (value < field->logical_minimum ||
+					value > field->logical_maximum) {
After thinking about it a little bit more, I think I agree.

[...]

Quoted 1 line

So please let me know the result of your testing.

Seems to work on my device, thanks!
I haven't done more than a couple of minutes of testing, though. But it seems to work.

(another test below)

I've also tested the behavior when one axis is valid, while the other one as an invalid (and thus discarded) value.

This was based on a question from Chris Friesen:
https://lkml.org/lkml/2011/10/24/290

Running "evtest /dev/input/event9" only shows the valid axis. That's good, it seems invalid values don't generate events.

But Xorg moves the pointer in both axes at once, instead of just one of them.
IMHO, that's not perfect, but it seems good enough, specially considering this is a somewhat unlikely edge-case (remember I'm talking about one axis valid and another discarded). It might also be a bug in Xorg.

(if anything in this message doesn't make much sense, that's probably because I need some sleep - but I wanted to give you feedback as soon as I could)

Signature · 2 lines

Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Jiri Kosina

2011-11-16 14:01 UTC #13 lore raw

Quoted 8 lines · Denilson Figueiredo de Sá

Quoted 4 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (value < field->logical_minimum ||
+					value > field->logical_maximum) {
After thinking about it a little bit more, I think I agree.
[...]
Quoted 1 line
So please let me know the result of your testing.
Seems to work on my device, thanks!

Thank you. I have now queued the patch with your Tested-by.

Signature · 6 lines

Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html