Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1263588 > unrolled thread

Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF Elias Vanderstuyft <elias.vds@gmail.com> - 2015-11-05 23:40 +0100
    Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and  EV_FF Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-11-06 02:40 +0100

#1263588 — Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF

Hi Dmitry,

Excuse me for the long delay.

On Thu, Oct 15, 2015 at 2:52 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Sep 17, 2015 at 07:29:48PM +0200, Elias Vanderstuyft wrote:
>> Currently the user can specify a non-zero value for ff_effects_max,
>> without setting the EV_FF bit.
>> Inversely,
>> the user can also set ff_effects_max to zero with the EV_FF bit set,
>> in this case the uninitialized method ff->upload can be dereferenced,
>> resulting in a kernel oops.
>>
>> Instead of adding a check in uinput_create_device() and
>> omitting setup of ff-core infrastructure silently in case the check fails,
>> perform the check early in uinput_setup_device(),
>> and print a helpful message and return -EINVAL in case the check fails.
>>
>> Signed-off-by: Elias Vanderstuyft <elias.vds@gmail.com>
>> ---
>>  drivers/input/misc/uinput.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index 345df9b..3a90a16 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -393,6 +393,21 @@ static int uinput_setup_device(struct uinput_device *udev,
>>       if (IS_ERR(user_dev))
>>               return PTR_ERR(user_dev);
>>
>> +     if (!!user_dev->ff_effects_max ^ test_bit(EV_FF, dev->evbit)) {
>> +             if (user_dev->ff_effects_max)
>> +                     printk(KERN_DEBUG
>> +                             "%s: ff_effects_max (%u) should be zero "
>> +                             "when FF_BIT is not set\n",
>> +                             UINPUT_NAME, user_dev->ff_effects_max);
>> +             else
>> +                     printk(KERN_DEBUG
>> +                             "%s: ff_effects_max should be non-zero "
>> +                             "when FF_BIT is set\n",
>> +                             UINPUT_NAME);
>
> I do not think this is the right place for this check: userspace is
> allowed to write device structure before calling any ioctls to set
> various bits. Also, userspace doe snot have to explicitly set EV_FF bit
> as input_ff_create() does it for us.

OK, I put it here to be consistent with the uinput_validate_absbits() function,
which checks absbit in case the EV_ABS bit is set,
but I incorrectly assumed the EV_ABS bit was required to be set.

> I think the check should be in uinput_create_device() and we should only
> check case when udev->ff_effects_max is 0 but EV_FF is set.

This made me think about the whole idea whether or not
allowing ff_effects_max to be zero is possible for a FF device.
I think it is perfectly possible to have a FF device with no support
for uploading effects,
but with an adjustable AUTOCENTER-force axis.
Or, more exotically, a device with a trigger-button which on press
automatically emits rumble,
with adjustable GAIN.

The only places where we'd need to change code for allowing this,
is in:
- http://lxr.free-electrons.com/source/drivers/input/ff-core.c?v=4.3#L316
    : remove if-then-block
- http://lxr.free-electrons.com/source/drivers/input/misc/uinput.c?v=4.3#L266
    : change if-test to "udev->ff_effects_max || test_bit(EV_FF, dev->evbit)"
Of course, the latter change may conflict with your initial reply:
userspace does not have to explicitly set the EV_FF bit in advance;
however it does make sense to set the bit if e.g. only FF_AUTOCENTER
support is available,
but no uploading of effects (ff->upload and friends will still be set,
but not used, thanks to check_effect_access()).

What do you think about this: should I go with "forbid ff_effects_max
to be zero, and check on it" or "allow ff_effects_max to be zero"?
My previous patches wouldn't conflict with either options.

Thanks,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1263686 — Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF

On Thu, Nov 05, 2015 at 11:34:55PM +0100, Elias Vanderstuyft wrote:
> Hi Dmitry,
> 
> Excuse me for the long delay.
> 
> On Thu, Oct 15, 2015 at 2:52 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Sep 17, 2015 at 07:29:48PM +0200, Elias Vanderstuyft wrote:
> >> Currently the user can specify a non-zero value for ff_effects_max,
> >> without setting the EV_FF bit.
> >> Inversely,
> >> the user can also set ff_effects_max to zero with the EV_FF bit set,
> >> in this case the uninitialized method ff->upload can be dereferenced,
> >> resulting in a kernel oops.
> >>
> >> Instead of adding a check in uinput_create_device() and
> >> omitting setup of ff-core infrastructure silently in case the check fails,
> >> perform the check early in uinput_setup_device(),
> >> and print a helpful message and return -EINVAL in case the check fails.
> >>
> >> Signed-off-by: Elias Vanderstuyft <elias.vds@gmail.com>
> >> ---
> >>  drivers/input/misc/uinput.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> index 345df9b..3a90a16 100644
> >> --- a/drivers/input/misc/uinput.c
> >> +++ b/drivers/input/misc/uinput.c
> >> @@ -393,6 +393,21 @@ static int uinput_setup_device(struct uinput_device *udev,
> >>       if (IS_ERR(user_dev))
> >>               return PTR_ERR(user_dev);
> >>
> >> +     if (!!user_dev->ff_effects_max ^ test_bit(EV_FF, dev->evbit)) {
> >> +             if (user_dev->ff_effects_max)
> >> +                     printk(KERN_DEBUG
> >> +                             "%s: ff_effects_max (%u) should be zero "
> >> +                             "when FF_BIT is not set\n",
> >> +                             UINPUT_NAME, user_dev->ff_effects_max);
> >> +             else
> >> +                     printk(KERN_DEBUG
> >> +                             "%s: ff_effects_max should be non-zero "
> >> +                             "when FF_BIT is set\n",
> >> +                             UINPUT_NAME);
> >
> > I do not think this is the right place for this check: userspace is
> > allowed to write device structure before calling any ioctls to set
> > various bits. Also, userspace doe snot have to explicitly set EV_FF bit
> > as input_ff_create() does it for us.
> 
> OK, I put it here to be consistent with the uinput_validate_absbits() function,
> which checks absbit in case the EV_ABS bit is set,
> but I incorrectly assumed the EV_ABS bit was required to be set.
> 
> > I think the check should be in uinput_create_device() and we should only
> > check case when udev->ff_effects_max is 0 but EV_FF is set.
> 
> This made me think about the whole idea whether or not
> allowing ff_effects_max to be zero is possible for a FF device.
> I think it is perfectly possible to have a FF device with no support
> for uploading effects,
> but with an adjustable AUTOCENTER-force axis.
> Or, more exotically, a device with a trigger-button which on press
> automatically emits rumble,
> with adjustable GAIN.
> 
> The only places where we'd need to change code for allowing this,
> is in:
> - http://lxr.free-electrons.com/source/drivers/input/ff-core.c?v=4.3#L316
>     : remove if-then-block
> - http://lxr.free-electrons.com/source/drivers/input/misc/uinput.c?v=4.3#L266
>     : change if-test to "udev->ff_effects_max || test_bit(EV_FF, dev->evbit)"
> Of course, the latter change may conflict with your initial reply:
> userspace does not have to explicitly set the EV_FF bit in advance;
> however it does make sense to set the bit if e.g. only FF_AUTOCENTER
> support is available,
> but no uploading of effects (ff->upload and friends will still be set,
> but not used, thanks to check_effect_access()).
> 
> What do you think about this: should I go with "forbid ff_effects_max
> to be zero, and check on it" or "allow ff_effects_max to be zero"?
> My previous patches wouldn't conflict with either options.

I do not think I ever saw a device that would not support any FF effects
but would need autocenter, so I'd disallowed max effects being 0.

By the way, there is a batch from David/Benjamin reworking the uinput
device setup, be mindful of it when adjusting your patch please.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web