Re: [PATCH v2 1/2] Input: evdev: propagate EVIOCSFF copy errors correctly

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but an Information Leak exists: Uninitialized kernel stack bytes from `struct ff_effect effect` are leaked to userspace.
--

commit bf36b6bd42a34ab4fa65c08221f5186ae11a4eac
Author: Iván Ezequiel Rodriguez <ivanrwcm25@gmail.com>

Input: evdev: propagate EVIOCSFF copy errors correctly

This commit updates evdev_do_ioctl() to return the actual error code from
input_ff_effect_from_user(), rather than always returning -EFAULT. This
ensures that incorrect buffer sizes correctly surface as -EINVAL.

> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c7325226cb866..405d43f840f3e 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -1175,8 +1175,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  		return str_to_user(dev->uniq, size, p);
>  
>  	case EVIOC_MASK_SIZE(EVIOCSFF):
> -		if (input_ff_effect_from_user(p, size, &effect))
> -			return -EFAULT;
> +		error = input_ff_effect_from_user(p, size, &effect);
> +		if (error)
> +			return error;

[Severity: Critical]
This is a pre-existing issue, but could this leak uninitialized stack
memory to userspace?

In evdev_do_ioctl(), the 48-byte effect structure is declared uninitialized
on the stack:

drivers/input/evdev.c:evdev_do_ioctl() {
    ...
    struct ff_effect effect;
    ...
}

If a 32-bit compat application calls the EVIOCSFF ioctl,
input_ff_effect_from_user() in compat mode copies only 44 bytes from
userspace into this structure:

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)))
    ...
}

This leaves the trailing 4 bytes of kernel stack memory in the effect
structure uninitialized.

The full 48-byte structure, including the uninitialized stack bytes, is
then copied into driver storage by input_ff_upload():

drivers/input/ff-core.c:input_ff_upload() {
    ...
		ff->effects[id] = *effect;
    ...
}

If the device is managed by uinput, a 64-bit userspace daemon handling
the virtual device can read the full 48-byte structure, capturing the
uninitialized kernel stack bytes and leaking them to userspace:

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

Could we initialize struct ff_effect effect to zero in evdev_do_ioctl()
to prevent this information leak?

>  
>  		error = input_ff_upload(dev, &effect, file);
>  		if (error)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831152156.166514-1-ivanrwcm25@gmail.com?part=1