* [PATCH 0/2] Input: Improve handling of ff max_effects
@ 2015-09-17 17:29 ` Elias Vanderstuyft
0 siblings, 0 replies; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-09-17 17:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-api, linux-kernel, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] Input: Improve handling of ff max_effects
@ 2015-09-17 17:29 ` Elias Vanderstuyft
0 siblings, 0 replies; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-09-17 17:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS
2015-09-17 17:29 ` Elias Vanderstuyft
(?)
@ 2015-09-17 17:29 ` Elias Vanderstuyft
2015-10-15 0:52 ` Dmitry Torokhov
-1 siblings, 1 reply; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-09-17 17:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-api, linux-kernel, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
@ 2015-09-17 17:29 ` Elias Vanderstuyft
0 siblings, 0 replies; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-09-17 17:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-api, linux-kernel, 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@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);
+ retval = -EINVAL;
+ goto exit;
+ }
+
udev->ff_effects_max = user_dev->ff_effects_max;
/* Ensure name is filled in */
--
1.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
@ 2015-09-17 17:29 ` Elias Vanderstuyft
0 siblings, 0 replies; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-09-17 17:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
2015-09-17 17:29 ` Elias Vanderstuyft
(?)
@ 2015-10-15 0:52 ` Dmitry Torokhov
2015-11-05 22:34 ` Elias Vanderstuyft
-1 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2015-10-15 0:52 UTC (permalink / raw)
To: Elias Vanderstuyft; +Cc: linux-input, linux-api, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS
2015-09-17 17:29 ` [PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS Elias Vanderstuyft
@ 2015-10-15 0:52 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2015-10-15 0:52 UTC (permalink / raw)
To: Elias Vanderstuyft; +Cc: linux-input, linux-api, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
2015-10-15 0:52 ` Dmitry Torokhov
@ 2015-11-05 22:34 ` Elias Vanderstuyft
0 siblings, 0 replies; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-11-05 22:34 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, linux-api, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
@ 2015-11-05 22:34 ` Elias Vanderstuyft
0 siblings, 0 replies; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-11-05 22:34 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: open list:HID CORE LAYER, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
@ 2015-11-06 1:32 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2015-11-06 1:32 UTC (permalink / raw)
To: Elias Vanderstuyft; +Cc: open list:HID CORE LAYER, linux-api, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
@ 2015-11-06 1:32 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2015-11-06 1:32 UTC (permalink / raw)
To: Elias Vanderstuyft
Cc: open list:HID CORE LAYER, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
2015-09-17 17:29 ` Elias Vanderstuyft
(?)
(?)
@ 2015-11-08 17:37 ` Elias Vanderstuyft
2015-12-19 1:50 ` Dmitry Torokhov
-1 siblings, 1 reply; 13+ messages in thread
From: Elias Vanderstuyft @ 2015-11-08 17:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, linux-api, linux-kernel, David Herrmann,
Benjamin Tissoires, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
2015-11-08 17:37 ` [PATCH v2 " Elias Vanderstuyft
@ 2015-12-19 1:50 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2015-12-19 1:50 UTC (permalink / raw)
To: Elias Vanderstuyft
Cc: linux-input, linux-api, linux-kernel, David Herrmann, Benjamin Tissoires
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-12-19 1:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-17 17:29 [PATCH 0/2] Input: Improve handling of ff max_effects Elias Vanderstuyft
2015-09-17 17:29 ` Elias Vanderstuyft
2015-09-17 17:29 ` [PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS Elias Vanderstuyft
2015-10-15 0:52 ` Dmitry Torokhov
2015-09-17 17:29 ` [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF Elias Vanderstuyft
2015-09-17 17:29 ` Elias Vanderstuyft
2015-10-15 0:52 ` Dmitry Torokhov
2015-11-05 22:34 ` Elias Vanderstuyft
2015-11-05 22:34 ` Elias Vanderstuyft
2015-11-06 1:32 ` Dmitry Torokhov
2015-11-06 1:32 ` Dmitry Torokhov
2015-11-08 17:37 ` [PATCH v2 " Elias Vanderstuyft
2015-12-19 1:50 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.