Re: [PATCH 1/3] input: uapi: Add trigger_left and trigger_right to ff_rumble_effect struct

[sashiko-bot@kernel.org]

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

New issues:
- [High] Adding `trigger_left` and `trigger_right` to `struct ff_rumble_effect` within a union breaks backward compatibility and causes random device actuation for legacy userspace applications.

Pre-existing issues:
- [High] 4 bytes of uninitialized kernel stack memory are leaked to userspace via the `uinput` subsystem when a 32-bit compat application sends an `EVIOCSFF` ioctl.
--

--- Patch [1]: [PATCH 1/3] input: uapi: Add trigger_left and trigger_right to ff_rumble_effect struct ---
commit 47b93f39a2f9b455e85b8381fa40a96ca5aaac36
Author: Daniel Bomar <dbdaniel42@gmail.com>

input: uapi: Add trigger_left and trigger_right to ff_rumble_effect struct

This adds two variables to control the trigger motors to the struct that
is passed in from userspace. It reuses existing union padding to maintain
the total size of struct ff_effect.

Link: https://lore.kernel.org/lkml/20220410220449.5071-1-dbdaniel42@gmail.com/

> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 6aa703fcfcfb7..3d62603a387b9 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
[ ... ]
> @@ -427,6 +429,8 @@ struct ff_periodic_effect {
>  struct ff_rumble_effect {
>  	__u16 strong_magnitude;
>  	__u16 weak_magnitude;
> +	__u16 trigger_left;
> +	__u16 trigger_right;
>  };

[Severity: High]
Does this break backward compatibility for legacy userspace applications?

Because struct ff_rumble_effect resides within a union inside struct ff_effect,
extending its size from 4 to 8 bytes does not change the overall size of
struct ff_effect, and the EVIOCSFF ioctl number remains identical.

Legacy userspace applications typically allocate struct ff_effect on the stack
without zero-initializing the entire union padding. When calling EVIOCSFF for
a rumble effect, they leave uninitialized stack memory in the trailing bytes
that now correspond to trigger_left and trigger_right.

Could an updated driver read these uninitialized fields and actuate the trigger
motors based on random stack garbage?

[Severity: High]
This isn't a bug introduced by this patch, but does the input subsystem leak
uninitialized kernel stack memory to userspace during 32-bit compat ioctls?

In drivers/input/evdev.c:evdev_do_ioctl(), an uninitialized 48-byte effect
variable is declared on the stack:

	struct ff_effect effect;

When a 32-bit compat application calls EVIOCSFF, input_ff_effect_from_user() in
drivers/input/input-compat.c copies only 44 bytes from userspace:

		if (copy_from_user(compat_effect, buffer,
				   sizeof(struct ff_effect_compat)))
			return -EFAULT;

This leaves 4 bytes of the effect structure untouched, containing uninitialized
kernel stack data.

If this effect is uploaded to a virtual uinput device, uinput_dev_upload_effect()
stores a pointer to this stack variable. When a 64-bit uinput daemon process
calls the UI_BEGIN_FF_UPLOAD ioctl in
drivers/input/misc/uinput.c:uinput_ioctl_handler():

		retval = uinput_ff_upload_to_user(p, &ff_up);

it copies the entire 48-byte struct ff_effect back to userspace, reliably
disclosing 4 bytes of uninitialized kernel stack memory per ioctl call. Could
this be exploited to read kernel memory layout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730095317.767418-1-guillaume.casal.42@gmail.com?part=1