Conversion between unsigned and signed for "ff_effects_max" in uinput and input.

Conversion between unsigned and signed for "ff_effects_max" in uinput and input.

[Elias Vanderstuyft]

Hi everyone,

Making observations based on the following headers:

uinput.h: "(struct uinput_device).ff_effects_max" is defined as "unsigned int".
uapi/uinput.h: "(struct uinput_user_dev).ff_effects_max" is defined as "__u32".

vs

input.h: "(struct ff_device).max_effects" is defined as "int",
however, signature of input_ff_create() in input.h is:
    int input_ff_create(struct input_dev *dev, unsigned int max_effects)

Why is "(struct ff_device).max_effects" defined as a signed integer,
instead of an unsigned integer, as defined in the majority of the headers?

If that question is cleared out,
I would like to point to a potential integer overflow in the assignment in
ff-core.c::input_ff_create():
    ff->max_effects = max_effects;
(http://lxr.free-electrons.com/source/drivers/input/ff-core.c#L337)
Although physically unlikely that max_effects would exceed 0x7FFFFFFF,
shouldn't there be a check to prevent "ff->max_effects" becoming negative?
Note that using UInput, the user can define its own value of max_effects,
so adding a check might be justified.

Feedback would be nice.

Thanks,
Elias

Re: Conversion between unsigned and signed for "ff_effects_max" in uinput and input.

[Dmitry Torokhov]

Hi Elias,

On Fri, Jul 17, 2015 at 12:09:37AM +0200, Elias Vanderstuyft wrote:
> Hi everyone,
> 
> Making observations based on the following headers:
> 
> uinput.h: "(struct uinput_device).ff_effects_max" is defined as "unsigned int".
> uapi/uinput.h: "(struct uinput_user_dev).ff_effects_max" is defined as "__u32".
> 
> vs
> 
> input.h: "(struct ff_device).max_effects" is defined as "int",
> however, signature of input_ff_create() in input.h is:
>     int input_ff_create(struct input_dev *dev, unsigned int max_effects)
> 
> Why is "(struct ff_device).max_effects" defined as a signed integer,
> instead of an unsigned integer, as defined in the majority of the headers?

The effect->id is signed (because we treat -1 as special value when
given to us by userspace) and so it made sense to have max_effects the
same type...

> 
> If that question is cleared out,
> I would like to point to a potential integer overflow in the assignment in
> ff-core.c::input_ff_create():
>     ff->max_effects = max_effects;
> (http://lxr.free-electrons.com/source/drivers/input/ff-core.c#L337)
> Although physically unlikely that max_effects would exceed 0x7FFFFFFF,
> shouldn't there be a check to prevent "ff->max_effects" becoming negative?
> Note that using UInput, the user can define its own value of max_effects,
> so adding a check might be justified.


Yes, we should compare against INT_MAX and fail with -EINVAL I guess.

Thanks.

-- 
Dmitry