Re: [PATCH 3/3] input: Add FF_TRIGGER_RUMBLE capability bit

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Extending `struct ff_rumble_effect` within the `ff_effect` union breaks backward compatibility for older userspace applications that use `EVIOCSFF` by exposing uninitialized memory as trigger magnitudes.
- [Low] Missing `FF_TRIGGER_RUMBLE` string representation in the `force` array in `hid-debug.c`.
- [Low] `FF_TRIGGER_RUMBLE` is not documented in the Force Feedback API documentation.
--

--- Patch [3]: [PATCH 3/3] input: Add FF_TRIGGER_RUMBLE capability bit ---
commit 454627a6a6ad091c91ae3a0477831268ea8fe178
Author: Guillaume Casal <[email protected]>

input: Add FF_TRIGGER_RUMBLE capability bit

This commit adds the FF_TRIGGER_RUMBLE capability bit to allow userspace to
discover if a device has motors under the triggers. The capability is added
as a read-only bit at 0x62 to avoid conflicting with the effect id space.

> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 3d62603a387b9..677e9505a4aa3 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -528,6 +528,7 @@ struct ff_effect {
>  
>  #define FF_GAIN		0x60
>  #define FF_AUTOCENTER	0x61
> +#define FF_TRIGGER_RUMBLE	0x62

[Severity: High]
Does extending struct ff_rumble_effect (as mentioned in the commit message)
cause a regression for older userspace applications using EVIOCSFF?

Legacy applications frequently allocate struct ff_effect on the stack,
initialize only known fields like strong_magnitude and weak_magnitude, and
pass the struct to the EVIOCSFF ioctl.

In drivers/input/evdev.c:evdev_do_ioctl():

    case EVIOC_MASK_SIZE(EVIOCSFF):
        if (input_ff_effect_from_user(p, size, &effect))
            return -EFAULT;

This copies the entire union size from userspace. If uninitialized stack
garbage is copied, it could be interpreted as trigger_left and trigger_right.

Then in drivers/input/ff-memless.c:ml_combine_effects():

    trigger_left = (u32)new->u.rumble.trigger_left * gain / 0xffff;
    trigger_right = (u32)new->u.rumble.trigger_right * gain / 0xffff;

These uninitialized fields are actively read and processed into the final
effect playback state. Could this result in random, unintended vibration of
the trigger motors?

[Severity: Low]
Is the FF_TRIGGER_RUMBLE string representation missing in the force array?

In drivers/hid/hid-debug.c:force:

    static const char *force[FF_CNT] = {
        [FF_RUMBLE] = "FF_RUMBLE",

Without adding [FF_TRIGGER_RUMBLE] = "FF_TRIGGER_RUMBLE" here,
names[EV_FF][0x62] evaluates to NULL. This causes hid_resolv_event() to fall
back to printing the raw hex value ('0062') instead of 'FF_TRIGGER_RUMBLE'
in debugfs.

[Severity: Low]
Is FF_TRIGGER_RUMBLE missing from the Force Feedback API documentation?

Documentation/input/ff.rst:Querying device capabilities lists supported
EV_FF capability bits:

    - FF_GAIN    gain is adjustable
    - FF_AUTOCENTER  autocenter is adjustable

Since this commit introduces a new capability, should it be documented here
so userspace developers know how to discover trigger rumble support?

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