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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Kernel stack memory leak via EVIOCSABS and EVIOCGABS ioctls.
- [High] Kernel stack memory leak and KMSAN uninit-value bug in EVIOCSFF ioctl for compat tasks.
--

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

Input: evdev: propagate EVIOCSFF copy errors correctly

Return the error from input_ff_effect_from_user() instead of always
mapping failures to -EFAULT, so wrong buffer sizes 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,
[ ... ]
>  	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;
>  
>  		error = input_ff_upload(dev, &effect, file);

[Severity: High]
This is a pre-existing issue, but can this leak kernel stack memory and
cause a use of uninitialized memory in compat mode?

In evdev_do_ioctl(), struct ff_effect effect is declared on the stack
without initialization. When a 32-bit compat userspace process calls the
EVIOCSFF ioctl, input_ff_effect_from_user() copies 44 bytes into the 48-byte
native struct, leaving 4 bytes uninitialized:

drivers/input/input-compat.c:input_ff_effect_from_user() {
    ...
    if (copy_from_user(compat_effect, buffer,
                       sizeof(struct ff_effect_compat)))
    ...
}

The kernel then copies the full 48-byte struct to the heap in
input_ff_upload():

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

If the input device is a uinput virtual device managed by a 64-bit daemon,
could the UI_BEGIN_FF_UPLOAD ioctl copy the full 48-byte struct to the
daemon, leaking the 4 bytes of uninitialized kernel stack memory?


While reviewing this, I noticed a similar pre-existing issue in the same
function with the EVIOCSABS and EVIOCGABS ioctls.

[Severity: High]
This is a pre-existing issue, but does this code leak kernel stack memory
to userspace?

In evdev_do_ioctl(), struct input_absinfo abs is declared without
initialization. A malicious user can provide a small size in the EVIOCSABS
ioctl command, causing only a partial copy and leaving the rest of the
struct uninitialized:

drivers/input/evdev.c:evdev_do_ioctl() {
    ...
    if (copy_from_user(&abs, p, min_t(size_t, size,
                                      sizeof(struct input_absinfo))))
    ...
}

The uninitialized stack memory is copied into the device's heap allocation:

drivers/input/evdev.c:evdev_do_ioctl() {
    ...
    dev->absinfo[t] = abs;
    ...
}

Can this memory subsequently be leaked to userspace when read back via the
EVIOCGABS ioctl?

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