[PATCH 0/2] Input: Improve handling of ff max_effects

[PATCH 0/2] Input: Improve handling of ff max_effects

[Elias Vanderstuyft]

This is a patch-set to improve the handling of max_effects in
ff-core and uinput.

Elias Vanderstuyft (2):
  Input: Document and check on implicitly defined FF_MAX_EFFECTS
  Input: uinput: Sanity check on ff_effects_max and EV_FF

 drivers/input/ff-core.c     |  5 +++++
 drivers/input/misc/uinput.c | 15 +++++++++++++++
 include/uapi/linux/input.h  |  8 ++++++++
 3 files changed, 28 insertions(+)

-- 
1.9.3

[PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS

[Elias Vanderstuyft]

There is an undocumented upper bound for the total number of ff effects:
    FF_GAIN (= 96).
This can be found as follows:
- user: write(EV_FF, effect_id, iterations)
    calls kernel: ff->playback(effect_id, ...): starts effect "effect_id"
- user: write(EV_FF, FF_GAIN, gain)
    calls kernel: ff->set_gain(gain, ...): sets gain

A collision occurs when effect_id equals FF_GAIN.
According to input_ff_event(),
FF_GAIN is the smallest value where a collision occurs.
Therefore the greatest safe value for effect_id is FF_GAIN - 1,
and thus the total number of effects should never exceed FF_GAIN.

Define FF_MAX_EFFECTS as FF_GAIN and check on this limit in ff-core.

Signed-off-by: Elias Vanderstuyft <elias.vds@gmail.com>
---
 drivers/input/ff-core.c    | 5 +++++
 include/uapi/linux/input.h | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index c642082..0bf7008 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -318,6 +318,11 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
 		return -EINVAL;
 	}
 
+	if (max_effects > FF_MAX_EFFECTS) {
+		dev_err(&dev->dev, "cannot allocate more than FF_MAX_EFFECTS effects\n");
+		return -EINVAL;
+	}
+
 	ff_dev_size = sizeof(struct ff_device) +
 				max_effects * sizeof(struct file *);
 	if (ff_dev_size < max_effects) /* overflow */
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 63915a7..42d7933 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -1200,6 +1200,14 @@ struct ff_effect {
 #define FF_GAIN		0x60
 #define FF_AUTOCENTER	0x61
 
+/*
+ * ff->playback(effect_id = FF_GAIN) is the first effect_id to
+ * cause a collision with another ff method, in this case ff->set_gain().
+ * Therefore the greatest safe value for effect_id is FF_GAIN - 1,
+ * and thus the total number of effects should never exceed FF_GAIN.
+ */
+#define FF_MAX_EFFECTS	FF_GAIN
+
 #define FF_MAX		0x7f
 #define FF_CNT		(FF_MAX+1)
 
-- 
1.9.3

Re: [PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS

[Dmitry Torokhov]

On Thu, Sep 17, 2015 at 07:29:47PM +0200, Elias Vanderstuyft wrote:
> There is an undocumented upper bound for the total number of ff effects:
>     FF_GAIN (= 96).
> This can be found as follows:
> - user: write(EV_FF, effect_id, iterations)
>     calls kernel: ff->playback(effect_id, ...): starts effect "effect_id"
> - user: write(EV_FF, FF_GAIN, gain)
>     calls kernel: ff->set_gain(gain, ...): sets gain
> 
> A collision occurs when effect_id equals FF_GAIN.
> According to input_ff_event(),
> FF_GAIN is the smallest value where a collision occurs.
> Therefore the greatest safe value for effect_id is FF_GAIN - 1,
> and thus the total number of effects should never exceed FF_GAIN.
> 
> Define FF_MAX_EFFECTS as FF_GAIN and check on this limit in ff-core.
> 
> Signed-off-by: Elias Vanderstuyft <elias.vds@gmail.com>

Applied, thank you.

> ---
>  drivers/input/ff-core.c    | 5 +++++
>  include/uapi/linux/input.h | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> index c642082..0bf7008 100644
> --- a/drivers/input/ff-core.c
> +++ b/drivers/input/ff-core.c
> @@ -318,6 +318,11 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
>  		return -EINVAL;
>  	}
>  
> +	if (max_effects > FF_MAX_EFFECTS) {
> +		dev_err(&dev->dev, "cannot allocate more than FF_MAX_EFFECTS effects\n");
> +		return -EINVAL;
> +	}
> +
>  	ff_dev_size = sizeof(struct ff_device) +
>  				max_effects * sizeof(struct file *);
>  	if (ff_dev_size < max_effects) /* overflow */
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 63915a7..42d7933 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -1200,6 +1200,14 @@ struct ff_effect {
>  #define FF_GAIN		0x60
>  #define FF_AUTOCENTER	0x61
>  
> +/*
> + * ff->playback(effect_id = FF_GAIN) is the first effect_id to
> + * cause a collision with another ff method, in this case ff->set_gain().
> + * Therefore the greatest safe value for effect_id is FF_GAIN - 1,
> + * and thus the total number of effects should never exceed FF_GAIN.
> + */
> +#define FF_MAX_EFFECTS	FF_GAIN
> +
>  #define FF_MAX		0x7f
>  #define FF_CNT		(FF_MAX+1)
>  
> -- 
> 1.9.3
> 

-- 
Dmitry

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

[Elias Vanderstuyft]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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);
+		retval = -EINVAL;
+		goto exit;
+	}
+
 	udev->ff_effects_max = user_dev->ff_effects_max;
 
 	/* Ensure name is filled in */
-- 
1.9.3

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

[Dmitry Torokhov]

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.

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.

Thanks.

-- 
Dmitry

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

[Elias Vanderstuyft]

Hi Dmitry,

Excuse me for the long delay.

On Thu, Oct 15, 2015 at 2:52 AM, Dmitry Torokhov
<dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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

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

[Dmitry Torokhov]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  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

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

[Elias Vanderstuyft]

Currently the user can set ff_effects_max to zero with the EV_FF bit
(and the FF_GAIN and/or FF_AUTOCENTER bits) set,
in this case the uninitialized methods
ff->set_gain and/or ff->set_autocenter can be dereferenced,
resulting in a kernel oops.

Check in uinput_create_device() and
print a helpful message and return -EINVAL in case the check fails.

Signed-off-by: Elias Vanderstuyft <elias.vds@gmail.com>
---
Changes in v2:
  - Rebase on pending patches from David Herrmann and Benjamin Tissoires:
    - v3 Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
    - Input: uinput - rework ABS validation
  - Don't require EV_FF bit to be set when ff_effects_max is non-zero
  - Move check from uinput_setup_device() to uinput_create_device()
  - Update commit description

At the same time, the new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctls were
tested as well (in both orders).
The legacy write() (instead of UINPUT_DEV_SETUP) was also tested.

 drivers/input/misc/uinput.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 1d93037..b9d0713 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -272,6 +272,13 @@ static int uinput_create_device(struct uinput_device *udev)
 		input_set_events_per_packet(dev, 60);
 	}
 
+	if (test_bit(EV_FF, dev->evbit) && !udev->ff_effects_max) {
+		printk(KERN_DEBUG "%s: ff_effects_max should be non-zero when FF_BIT is set\n",
+			UINPUT_NAME);
+		error = -EINVAL;
+		goto fail1;
+	}
+
 	if (udev->ff_effects_max) {
 		error = input_ff_create(dev, udev->ff_effects_max);
 		if (error)
-- 
1.9.3

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

[Dmitry Torokhov]

On Sun, Nov 08, 2015 at 06:37:34PM +0100, Elias Vanderstuyft wrote:
> Currently the user can set ff_effects_max to zero with the EV_FF bit
> (and the FF_GAIN and/or FF_AUTOCENTER bits) set,
> in this case the uninitialized methods
> ff->set_gain and/or ff->set_autocenter can be dereferenced,
> resulting in a kernel oops.
> 
> Check in uinput_create_device() and
> print a helpful message and return -EINVAL in case the check fails.
> 
> Signed-off-by: Elias Vanderstuyft <elias.vds@gmail.com>

Applied, thank you.

> ---
> Changes in v2:
>   - Rebase on pending patches from David Herrmann and Benjamin Tissoires:
>     - v3 Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
>     - Input: uinput - rework ABS validation
>   - Don't require EV_FF bit to be set when ff_effects_max is non-zero
>   - Move check from uinput_setup_device() to uinput_create_device()
>   - Update commit description
> 
> At the same time, the new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctls were
> tested as well (in both orders).
> The legacy write() (instead of UINPUT_DEV_SETUP) was also tested.
> 
>  drivers/input/misc/uinput.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 1d93037..b9d0713 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -272,6 +272,13 @@ static int uinput_create_device(struct uinput_device *udev)
>  		input_set_events_per_packet(dev, 60);
>  	}
>  
> +	if (test_bit(EV_FF, dev->evbit) && !udev->ff_effects_max) {
> +		printk(KERN_DEBUG "%s: ff_effects_max should be non-zero when FF_BIT is set\n",
> +			UINPUT_NAME);
> +		error = -EINVAL;
> +		goto fail1;
> +	}
> +
>  	if (udev->ff_effects_max) {
>  		error = input_ff_create(dev, udev->ff_effects_max);
>  		if (error)
> -- 
> 1.9.3
> 

-- 
Dmitry