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

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

Pre-existing issues:
- [High] Kernel stack memory leak via `evdev` compat IOCTL and `uinput`.
--

--- Patch [1]: [PATCH v2 1/4] input: Add FF_TRIGGER_RUMBLE effect type ---
commit 2bed919a335670e21f97b67d6c9c64308176a631
Author: Guillaume Casal <[email protected]>
Subject: input: Add FF_TRIGGER_RUMBLE effect type

This commit adds a new FF_TRIGGER_RUMBLE force feedback effect type to support
impulse triggers found on modern gamepads. It introduces the
ff_trigger_rumble_effect structure and adds it to the ff_effect union. This
avoids ABI compatibility issues that could arise from reusing existing structs
with uninitialized tail bytes.

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

[ ... ]

> @@ -483,6 +497,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;
>  		struct ff_haptic_effect haptic;
>  	} u;
>  };

[Severity: High]
This is a pre-existing issue, but could the handling of EVIOCSFF via compat
ioctl lead to a kernel stack memory leak when used with uinput?

When a 32-bit userspace client calls the EVIOCSFF ioctl on an evdev device
backed by a uinput device, evdev_do_ioctl() allocates a 48-byte
struct ff_effect on the kernel stack without initialization:

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))
            return -EFAULT;

        error = input_ff_upload(dev, &effect, file);
    ...
}

For compat applications, input_ff_effect_from_user() only copies 44 bytes
(sizeof(struct ff_effect_compat)), leaving the last 4 bytes of the effect
struct uninitialized:

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 a 64-bit userspace daemon retrieves the force-feedback effect using the
UI_BEGIN_FF_UPLOAD ioctl, uinput_ioctl_handler() copies the full 48-byte
structure back to userspace:

drivers/input/misc/uinput.c:uinput_ioctl_handler() {
    ff_up.effect = *req->u.upload.effect;
    ...
    retval = uinput_ff_upload_to_user(p, &ff_up);
    ...
}

Does this sequence allow the 64-bit uinput daemon to read 4 bytes of
uninitialized kernel stack data?

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