[PATCH 1/1] HID: clamp input to logical range if no null state

[PATCH 1/1] HID: clamp input to logical range if no null state

[Tomasz Kramkowski]

This patch fixes an issue in drivers/hid/hid-input.c where values
outside of the logical range are not clamped when "null state" bit of
the input control is not set.

This was discussed on the lists [1] and this change stems from the fact
due to the ambiguity of the HID specification it might be appropriate to
follow Microsoft's own interpretation of the specification. As noted in
Microsoft's documentation [2] in the section titled "Required HID usages
for digitizers" it is noted that values reported outside the logical
range "will be considered as invalid data and the value will be changed
to the nearest boundary value (logical min/max)."

This patch fixes an issue where the (1292:4745) Innomedia INNEX
GENESIS/ATARI reports out of range values for its X and Y axis of the
DPad which, due to the null state bit being unset, are forwarded to
userspace as is. Now these values will get clamped to the logical range
before being forwarded to userspace. This device was also used to test
this patch.

This patch expands on commit 3f3752705dbd ("HID: reject input outside
logical range only if null state is set").

[1]: http://lkml.kernel.org/r/20170307131036.GA853@gaia.local
[2]: https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp

Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
 drivers/hid/hid-input.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index cf8256aac2bd..cf38ff79cfe9 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1157,12 +1157,15 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	 * don't specify logical min and max.
 	 */
 	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
-	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
 	    (field->logical_minimum < field->logical_maximum) &&
 	    (value < field->logical_minimum ||
 	     value > field->logical_maximum)) {
-		dbg_hid("Ignoring out-of-range value %x\n", value);
-		return;
+		if (field->flags & HID_MAIN_ITEM_NULL_STATE) {
+			dbg_hid("Ignoring out-of-range value %x\n", value);
+			return;
+		}
+		value = value < field->logical_minimum ?
+			field->logical_minimum : field->logical_maximum;
 	}
 
 	/*
-- 
2.12.0

Re: [PATCH 1/1] HID: clamp input to logical range if no null state

[Benjamin Tissoires]

On Mar 08 2017 or thereabouts, Tomasz Kramkowski wrote:
> This patch fixes an issue in drivers/hid/hid-input.c where values
> outside of the logical range are not clamped when "null state" bit of
> the input control is not set.
> 
> This was discussed on the lists [1] and this change stems from the fact
> due to the ambiguity of the HID specification it might be appropriate to
> follow Microsoft's own interpretation of the specification. As noted in
> Microsoft's documentation [2] in the section titled "Required HID usages
> for digitizers" it is noted that values reported outside the logical
> range "will be considered as invalid data and the value will be changed
> to the nearest boundary value (logical min/max)."
> 
> This patch fixes an issue where the (1292:4745) Innomedia INNEX
> GENESIS/ATARI reports out of range values for its X and Y axis of the
> DPad which, due to the null state bit being unset, are forwarded to
> userspace as is. Now these values will get clamped to the logical range
> before being forwarded to userspace. This device was also used to test
> this patch.
> 
> This patch expands on commit 3f3752705dbd ("HID: reject input outside
> logical range only if null state is set").
> 
> [1]: http://lkml.kernel.org/r/20170307131036.GA853@gaia.local
> [2]: https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
> 
> Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
> ---
>  drivers/hid/hid-input.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index cf8256aac2bd..cf38ff79cfe9 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1157,12 +1157,15 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>  	 * don't specify logical min and max.
>  	 */
>  	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
> -	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
>  	    (field->logical_minimum < field->logical_maximum) &&
>  	    (value < field->logical_minimum ||
>  	     value > field->logical_maximum)) {
> -		dbg_hid("Ignoring out-of-range value %x\n", value);
> -		return;
> +		if (field->flags & HID_MAIN_ITEM_NULL_STATE) {
> +			dbg_hid("Ignoring out-of-range value %x\n", value);
> +			return;
> +		}
> +		value = value < field->logical_minimum ?
> +			field->logical_minimum : field->logical_maximum;

We have a "clamp()" function in the kernel that does the job directly
and which is more readable. Also, this makes testing the out of range
values twice.

How about:

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index cf8256a..781f400 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 
 	/*
 	 * Ignore out-of-range values as per HID specification,
-	 * section 5.10 and 6.2.25.
+	 * section 5.10 and 6.2.25, when NULL state bit is present.
+	 * When it's not, clamp the value to match Microsoft's input
+	 * driver as mentioned in "Required HID usages for digitizers":
+	 * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
 	 *
 	 * The logical_minimum < logical_maximum check is done so that we
 	 * don't unintentionally discard values sent by devices which
 	 * don't specify logical min and max.
 	 */
 	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
-	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
-	    (field->logical_minimum < field->logical_maximum) &&
-	    (value < field->logical_minimum ||
-	     value > field->logical_maximum)) {
-		dbg_hid("Ignoring out-of-range value %x\n", value);
-		return;
+	    (field->logical_minimum < field->logical_maximum)) {
+		if (!(field->flags & HID_MAIN_ITEM_NULL_STATE)) {
+			value = clamp(value,
+				      field->logical_minimum,
+				      field->logical_maximum);
+		} else if (value < field->logical_minimum ||
+			   value > field->logical_maximum) {
+			dbg_hid("Ignoring out-of-range value %x\n", value);
+			return;
+		}
 	}
 
 	/*
---

Cheers,
Benjamin

>  	}
>  
>  	/*
> -- 
> 2.12.0
> 

Re: [PATCH 1/1] HID: clamp input to logical range if no null state

[Tomasz Kramkowski]

On Thu, Mar 09, 2017 at 09:16:06AM +0100, Benjamin Tissoires wrote:
> We have a "clamp()" function in the kernel that does the job directly
> and which is more readable. Also, this makes testing the out of range
> values twice.
> 
> How about:
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index cf8256a..781f400 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>  
>  	/*
>  	 * Ignore out-of-range values as per HID specification,
> -	 * section 5.10 and 6.2.25.
> +	 * section 5.10 and 6.2.25, when NULL state bit is present.
> +	 * When it's not, clamp the value to match Microsoft's input
> +	 * driver as mentioned in "Required HID usages for digitizers":
> +	 * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
>  	 *
>  	 * The logical_minimum < logical_maximum check is done so that we
>  	 * don't unintentionally discard values sent by devices which
>  	 * don't specify logical min and max.
>  	 */
>  	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
> -	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
> -	    (field->logical_minimum < field->logical_maximum) &&
> -	    (value < field->logical_minimum ||
> -	     value > field->logical_maximum)) {
> -		dbg_hid("Ignoring out-of-range value %x\n", value);
> -		return;
> +	    (field->logical_minimum < field->logical_maximum)) {
>  	}

Yes, I don't mind the expansion of the comment and the usage of clamp (I
didn't know this existed, but I will use it in the future). However if
there is anything I would change, it would be this:

---
 drivers/hid/hid-input.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index cf8256aac2bd..a1ebdd7d4d4d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 
 	/*
 	 * Ignore out-of-range values as per HID specification,
-	 * section 5.10 and 6.2.25.
+	 * section 5.10 and 6.2.25, when NULL state bit is present.
+	 * When it's not, clamp the value to match Microsoft's input
+	 * driver as mentioned in "Required HID usages for digitizers":
+	 * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
 	 *
 	 * The logical_minimum < logical_maximum check is done so that we
 	 * don't unintentionally discard values sent by devices which
 	 * don't specify logical min and max.
 	 */
 	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
-	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
-	    (field->logical_minimum < field->logical_maximum) &&
-	    (value < field->logical_minimum ||
-	     value > field->logical_maximum)) {
-		dbg_hid("Ignoring out-of-range value %x\n", value);
-		return;
+	    (field->logical_minimum < field->logical_maximum)) {
+		if (field->flags & HID_MAIN_ITEM_NULL_STATE &&
+		    (value < field->logical_minimum ||
+		     value > field->logical_maximum)) {
+			dbg_hid("Ignoring out-of-range value %x\n", value);
+			return;
+		}
+		value = clamp(value,
+			      field->logical_minimum,
+			      field->logical_maximum);
 	}
 
 	/*
-- 
2.12.0

For me it is a bit clearer on what is happening and still avoids doing
the range check twice. But ultimately it is all up to you guys.

I can get both versions of this patch tested at some point in the next
few days and re-submit whichever one you prefer as a v2.

I'm not sure what the procedures are on this, should I put a
"Suggested-by:" for your suggested change to my patch, or is that not
applicable here?

As always, thanks for your time.

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

Re: [PATCH 1/1] HID: clamp input to logical range if no null state

[Benjamin Tissoires]

On Mar 12 2017 or thereabouts, Tomasz Kramkowski wrote:
> On Thu, Mar 09, 2017 at 09:16:06AM +0100, Benjamin Tissoires wrote:
> > We have a "clamp()" function in the kernel that does the job directly
> > and which is more readable. Also, this makes testing the out of range
> > values twice.
> > 
> > How about:
> > 
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index cf8256a..781f400 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
> >  
> >  	/*
> >  	 * Ignore out-of-range values as per HID specification,
> > -	 * section 5.10 and 6.2.25.
> > +	 * section 5.10 and 6.2.25, when NULL state bit is present.
> > +	 * When it's not, clamp the value to match Microsoft's input
> > +	 * driver as mentioned in "Required HID usages for digitizers":
> > +	 * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
> >  	 *
> >  	 * The logical_minimum < logical_maximum check is done so that we
> >  	 * don't unintentionally discard values sent by devices which
> >  	 * don't specify logical min and max.
> >  	 */
> >  	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
> > -	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
> > -	    (field->logical_minimum < field->logical_maximum) &&
> > -	    (value < field->logical_minimum ||
> > -	     value > field->logical_maximum)) {
> > -		dbg_hid("Ignoring out-of-range value %x\n", value);
> > -		return;
> > +	    (field->logical_minimum < field->logical_maximum)) {
> >  	}
> 
> Yes, I don't mind the expansion of the comment and the usage of clamp (I
> didn't know this existed, but I will use it in the future). However if
> there is anything I would change, it would be this:
> 
> ---
>  drivers/hid/hid-input.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index cf8256aac2bd..a1ebdd7d4d4d 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>  
>  	/*
>  	 * Ignore out-of-range values as per HID specification,
> -	 * section 5.10 and 6.2.25.
> +	 * section 5.10 and 6.2.25, when NULL state bit is present.
> +	 * When it's not, clamp the value to match Microsoft's input
> +	 * driver as mentioned in "Required HID usages for digitizers":
> +	 * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
>  	 *
>  	 * The logical_minimum < logical_maximum check is done so that we
>  	 * don't unintentionally discard values sent by devices which
>  	 * don't specify logical min and max.
>  	 */
>  	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
> -	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
> -	    (field->logical_minimum < field->logical_maximum) &&
> -	    (value < field->logical_minimum ||
> -	     value > field->logical_maximum)) {
> -		dbg_hid("Ignoring out-of-range value %x\n", value);
> -		return;
> +	    (field->logical_minimum < field->logical_maximum)) {
> +		if (field->flags & HID_MAIN_ITEM_NULL_STATE &&
> +		    (value < field->logical_minimum ||
> +		     value > field->logical_maximum)) {
> +			dbg_hid("Ignoring out-of-range value %x\n", value);
> +			return;
> +		}
> +		value = clamp(value,
> +			      field->logical_minimum,
> +			      field->logical_maximum);
>  	}
>  
>  	/*
> -- 
> 2.12.0
> 
> For me it is a bit clearer on what is happening and still avoids doing
> the range check twice. But ultimately it is all up to you guys.

Works for me.

> 
> I can get both versions of this patch tested at some point in the next
> few days and re-submit whichever one you prefer as a v2.
> 
> I'm not sure what the procedures are on this, should I put a
> "Suggested-by:" for your suggested change to my patch, or is that not
> applicable here?

No need to add suggested-by. This tag, IMO, is there to give credit on
an idea, while here it was more a common effort :)

Just add my Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
if you resubmit it - after the tests :)

Cheers,
Benjamin


> 
> As always, thanks for your time.
> 
> -- 
> Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/