[PATCH] Input :Added Check for EV_ABS event params

[PATCH] Input :Added Check for EV_ABS event params

[Anshul Garg]

From: Anshul Garg <aksgarg1989@gmail.com>

while handling EV_ABS event in input_handle_abs_event
function added check for out of range event value from
input driver. As input driver sets the ABS params at
registration time so input core should ignore events out
of the range set by the input driver.

Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
---
 drivers/input/input.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index cc357f1..b1a6ff6 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -244,6 +244,11 @@ static int input_handle_abs_event(struct input_dev *dev,
 		pold = NULL;
 	}
 
+	if (dev->absinfo[code].minimum > *pval || dev->absinfo[code].maximum < *pval) {
+		/* Ignore event with out of range values */		
+		return INPUT_IGNORE_EVENT;
+	}
+
 	if (pold) {
 		*pval = input_defuzz_abs_event(*pval, *pold,
 						dev->absinfo[code].fuzz);
-- 
1.7.9.5


---
This email has been checked for viruses by Avast antivirus software.
http://www.avast.com

Re: [PATCH] Input :Added Check for EV_ABS event params

[Dmitry Torokhov]

Hi Anshul,

On Tue, Apr 21, 2015 at 11:19:52AM -0700, Anshul Garg wrote:
> From: Anshul Garg <aksgarg1989@gmail.com>
> 
> while handling EV_ABS event in input_handle_abs_event
> function added check for out of range event value from
> input driver. As input driver sets the ABS params at
> registration time so input core should ignore events out
> of the range set by the input driver.

No, I do not think we want to do that, at least not unconditionally,
especially since it is perfectly allowed to use 0 as min/max, which
means that exact min and max are not defined. Historically min and max
were provided to the userspace as a guidance and it was up to userspace
to decide what to do with values outside of the limits.

Thanks.

> 
> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
> ---
>  drivers/input/input.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index cc357f1..b1a6ff6 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -244,6 +244,11 @@ static int input_handle_abs_event(struct input_dev *dev,
>  		pold = NULL;
>  	}
>  
> +	if (dev->absinfo[code].minimum > *pval || dev->absinfo[code].maximum < *pval) {
> +		/* Ignore event with out of range values */		
> +		return INPUT_IGNORE_EVENT;
> +	}
> +
>  	if (pold) {
>  		*pval = input_defuzz_abs_event(*pval, *pold,
>  						dev->absinfo[code].fuzz);
> -- 
> 1.7.9.5
> 
> 
> ---
> This email has been checked for viruses by Avast antivirus software.
> http://www.avast.com
> 

-- 
Dmitry

Re: [PATCH] Input :Added Check for EV_ABS event params

[Peter Hutterer]

On Tue, Apr 21, 2015 at 11:29:16AM -0700, Dmitry Torokhov wrote:
> Hi Anshul,
> 
> On Tue, Apr 21, 2015 at 11:19:52AM -0700, Anshul Garg wrote:
> > From: Anshul Garg <aksgarg1989@gmail.com>
> > 
> > while handling EV_ABS event in input_handle_abs_event
> > function added check for out of range event value from
> > input driver. As input driver sets the ABS params at
> > registration time so input core should ignore events out
> > of the range set by the input driver.
> 
> No, I do not think we want to do that, at least not unconditionally,
> especially since it is perfectly allowed to use 0 as min/max, which
> means that exact min and max are not defined. Historically min and max
> were provided to the userspace as a guidance and it was up to userspace
> to decide what to do with values outside of the limits.

for the archives:
X requires us to drop coordinates outside the given range, at least the 
ones we forward to the clients from absolute devices. We have used
out-of-bounds variables within the synaptics driver in the past though.

Cheers,
   Peter

> > 
> > Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
> > ---
> >  drivers/input/input.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index cc357f1..b1a6ff6 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -244,6 +244,11 @@ static int input_handle_abs_event(struct input_dev *dev,
> >  		pold = NULL;
> >  	}
> >  
> > +	if (dev->absinfo[code].minimum > *pval || dev->absinfo[code].maximum < *pval) {
> > +		/* Ignore event with out of range values */		
> > +		return INPUT_IGNORE_EVENT;
> > +	}
> > +
> >  	if (pold) {
> >  		*pval = input_defuzz_abs_event(*pval, *pold,
> >  						dev->absinfo[code].fuzz);
> > -- 
> > 1.7.9.5
> > 

Re: [PATCH] Input :Added Check for EV_ABS event params

[Hans de Goede]

Hi,

On 21-04-15 23:56, Peter Hutterer wrote:
> On Tue, Apr 21, 2015 at 11:29:16AM -0700, Dmitry Torokhov wrote:
>> Hi Anshul,
>>
>> On Tue, Apr 21, 2015 at 11:19:52AM -0700, Anshul Garg wrote:
>>> From: Anshul Garg <aksgarg1989@gmail.com>
>>>
>>> while handling EV_ABS event in input_handle_abs_event
>>> function added check for out of range event value from
>>> input driver. As input driver sets the ABS params at
>>> registration time so input core should ignore events out
>>> of the range set by the input driver.
>>
>> No, I do not think we want to do that, at least not unconditionally,
>> especially since it is perfectly allowed to use 0 as min/max, which
>> means that exact min and max are not defined. Historically min and max
>> were provided to the userspace as a guidance and it was up to userspace
>> to decide what to do with values outside of the limits.
>
> for the archives:
> X requires us to drop coordinates outside the given range, at least the
> ones we forward to the clients from absolute devices. We have used
> out-of-bounds variables within the synaptics driver in the past though.

Also for the record, for touchpad devices min-max typically give the edges
not the absolute min / max values of the sensor. In some cases there may also
be margins given in the min/max values coming from the touchpad-firmware which
already take an edge-scrolling area into account. So with touchpads getting
values outside of the min/max range is sometimes something which not only
may happen, but actually is expected to happen.

Regards,

Hans

Re: [PATCH] Input :Added Check for EV_ABS event params

[Anshul Garg]

Hello Mr. Dmitry ,

On Tue, Apr 21, 2015 at 11:59 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Anshul,
>
> On Tue, Apr 21, 2015 at 11:19:52AM -0700, Anshul Garg wrote:
>> From: Anshul Garg <aksgarg1989@gmail.com>
>>
>> while handling EV_ABS event in input_handle_abs_event
>> function added check for out of range event value from
>> input driver. As input driver sets the ABS params at
>> registration time so input core should ignore events out
>> of the range set by the input driver.
>
> No, I do not think we want to do that, at least not unconditionally,
> especially since it is perfectly allowed to use 0 as min/max, which
> means that exact min and max are not defined. Historically min and max
> were provided to the userspace as a guidance and it was up to userspace
> to decide what to do with values outside of the limits.
>
> Thanks.

Thanks for the feedback.

Yes this check should not be added unconditionally as by default
min,max value will be zero.
If input driver has called input_set_abs_params function it means
both max,min will not be zero.

So we can add this check if at least one from min,max is non zero.

Moreover as per my understanding input driver will use this function
if it wants to control range of event value to be sent to user space or in case
fuzz logic to be applied on events sent from input driver.

Yes we can leave this decision on user space  but if out of range event
can be ignored at kernel layer it will save some operation from driver
perspective.

Thanks


>
>>
>> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
>> ---
>>  drivers/input/input.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index cc357f1..b1a6ff6 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -244,6 +244,11 @@ static int input_handle_abs_event(struct input_dev *dev,
>>               pold = NULL;
>>       }
>>
>> +     if (dev->absinfo[code].minimum > *pval || dev->absinfo[code].maximum < *pval) {
>> +             /* Ignore event with out of range values */
>> +             return INPUT_IGNORE_EVENT;
>> +     }
>> +
>>       if (pold) {
>>               *pval = input_defuzz_abs_event(*pval, *pold,
>>                                               dev->absinfo[code].fuzz);
>> --
>> 1.7.9.5
>>
>>
>> ---
>> This email has been checked for viruses by Avast antivirus software.
>> http://www.avast.com
>>
>
> --
> Dmitry

Re: [PATCH] Input :Added Check for EV_ABS event params

[Benjamin Tissoires]

On Wed, Apr 22, 2015 at 9:44 AM, Anshul Garg <aksgarg1989@gmail.com> wrote:
> Hello Mr. Dmitry ,
>
> On Tue, Apr 21, 2015 at 11:59 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> Hi Anshul,
>>
>> On Tue, Apr 21, 2015 at 11:19:52AM -0700, Anshul Garg wrote:
>>> From: Anshul Garg <aksgarg1989@gmail.com>
>>>
>>> while handling EV_ABS event in input_handle_abs_event
>>> function added check for out of range event value from
>>> input driver. As input driver sets the ABS params at
>>> registration time so input core should ignore events out
>>> of the range set by the input driver.
>>
>> No, I do not think we want to do that, at least not unconditionally,
>> especially since it is perfectly allowed to use 0 as min/max, which
>> means that exact min and max are not defined. Historically min and max
>> were provided to the userspace as a guidance and it was up to userspace
>> to decide what to do with values outside of the limits.
>>
>> Thanks.
>
> Thanks for the feedback.
>
> Yes this check should not be added unconditionally as by default
> min,max value will be zero.
> If input driver has called input_set_abs_params function it means
> both max,min will not be zero.
>
> So we can add this check if at least one from min,max is non zero.
>
> Moreover as per my understanding input driver will use this function
> if it wants to control range of event value to be sent to user space or in case
> fuzz logic to be applied on events sent from input driver.
>
> Yes we can leave this decision on user space  but if out of range event
> can be ignored at kernel layer it will save some operation from driver
> perspective.
>

As Hans said, this won't do. In addition to Synaptics, which uses out
of bound values for a long time, the Wacom driver is doing this too.
This is *very* useful when the stylus is slightly out of the screen to
get access to the last/first row/column of pixel.

Returning IGNORE_EVENT will just break existing hardware even if the
driver sets proper min/max.

Xorg and libinput just clamp the values (which is different to
ignore), but I can easily think of scenarios where the user gets
feedback even out of the screen.

One more thing: the HID subsystem already does that for HID devices.
That is only because the HID specification says so. And this covers
most of the devices available.
So if you have a problem with a specific hardware, fix the driver,
that would be the easiest way to do, and you will be sure to not break
any other devices.

Cheers,
Benjamin

>
>>
>>>
>>> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
>>> ---
>>>  drivers/input/input.c |    5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>> index cc357f1..b1a6ff6 100644
>>> --- a/drivers/input/input.c
>>> +++ b/drivers/input/input.c
>>> @@ -244,6 +244,11 @@ static int input_handle_abs_event(struct input_dev *dev,
>>>               pold = NULL;
>>>       }
>>>
>>> +     if (dev->absinfo[code].minimum > *pval || dev->absinfo[code].maximum < *pval) {
>>> +             /* Ignore event with out of range values */
>>> +             return INPUT_IGNORE_EVENT;
>>> +     }
>>> +
>>>       if (pold) {
>>>               *pval = input_defuzz_abs_event(*pval, *pold,
>>>                                               dev->absinfo[code].fuzz);
>>> --
>>> 1.7.9.5
>>>
>>>
>>> ---
>>> This email has been checked for viruses by Avast antivirus software.
>>> http://www.avast.com
>>>
>>
>> --
>> Dmitry