Re: [PATCH v3 1/4] input: Add FF_TRIGGER_RUMBLE effect type

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The memoryless force-feedback core (`ff-memless.c`) lacks support for the newly added `FF_TRIGGER_RUMBLE` effect, causing trigger rumble effects to be silently discarded.
- [Low] The `FF_TRIGGER_RUMBLE` effect is missing from the HID debug subsystem's string mapping array.

Pre-existing issues:
- [High] Pre-existing 4-byte kernel stack memory leak to userspace in 32-bit compat EVIOCSFF ioctl via partially uninitialized struct ff_effect.
--

--- Patch [1]: [PATCH v3 1/4] input: Add FF_TRIGGER_RUMBLE effect type ---
commit 3d2db8431a4c26c55eb87cf71c1052fd6e1eefd5
Author: Guillaume Casal <[email protected]>

input: Add FF_TRIGGER_RUMBLE effect type

Some gamepads carry rumble motors behind their triggers, in addition to
the ones in the grips that FF_RUMBLE drives: Xbox controllers call them
impulse triggers, and handhelds such as the ASUS ROG Xbox Ally X have
them too. The force feedback API cannot reach those motors.

[ ... ]

Based on an earlier attempt by Daniel Bomar:

  https://lore.kernel.org/lkml/[email protected]/

> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 6aa703fcfcfb7..e63d19d9a093d 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h

[ ... ]

> @@ -483,6 +501,7 @@ struct ff_effect {
>  		struct ff_periodic_effect periodic;
>  		struct ff_condition_effect condition[2]; /* One for each axis */
>  		struct ff_rumble_effect rumble;
> +		struct ff_trigger_rumble_effect trigger_rumble;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but does
adding to this union highlight a potential kernel stack memory leak when
struct ff_effect is used in the 32-bit compat EVIOCSFF ioctl?

When evdev_do_ioctl() in drivers/input/evdev.c declares the struct on
the stack without zero-initializing it:

drivers/input/evdev.c:evdev_do_ioctl() {
        struct ff_effect effect;
        ...
        case EVIOC_MASK_SIZE(EVIOCSFF):
                if (input_ff_effect_from_user(p, size, &effect))

Does the compat handler in drivers/input/input-compat.c leave the last
4 bytes of the 48-byte struct uninitialized by only copying 44 bytes?

drivers/input/input-compat.c:input_ff_effect_from_user() {
        compat_effect = (struct ff_effect_compat *)effect;

        if (copy_from_user(compat_effect, buffer,
                           sizeof(struct ff_effect_compat)))

If the evdev device is backed by uinput, can those uninitialized stack
bytes then be copied to the 64-bit uinput daemon?

drivers/input/misc/uinput.c:uinput_ff_upload_to_user() {
        if (copy_to_user(buffer, ff_up, sizeof(struct uinput_ff_upload)))

>  		struct ff_haptic_effect haptic;
>  	} u;
>  };
> @@ -491,6 +510,7 @@ struct ff_effect {
>   * Force feedback effect types
>   */
>  
> +#define FF_TRIGGER_RUMBLE	0x4e

[Severity: Medium]
Does the memoryless force-feedback core need to be updated to support
this new effect?

Looking at ml_combine_effects() in drivers/input/ff-memless.c, it
appears FF_TRIGGER_RUMBLE will fall through to the default case and log
an error instead of populating the combined effect magnitudes:

drivers/input/ff-memless.c:ml_combine_effects() {
        switch (new->type) {
        case FF_CONSTANT:
        ...
        default:
                pr_err("invalid type in ml_combine_effects()\n");

[Severity: Low]
Should this new effect type also be added to the string mapping array in
the HID debug subsystem?

Looking at drivers/hid/hid-debug.c, debugfs reads will fail to display
the string name for FF_TRIGGER_RUMBLE without it:

drivers/hid/hid-debug.c:
static const char *force[FF_CNT] = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1