Re: [PATCH] Input: uinput - fix use-after-free of FF requests in ioctl handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] input: uinput: data race on request->retval in submitter
--

commit 8d5b9600ec78cd8cecd500a481ecb1dea1d928da
Author: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>

Input: uinput - fix use-after-free of FF requests in ioctl handlers

This commit fixes a use-after-free issue in uinput ioctl handlers by holding
requests_lock during request lookup and usage. It introduces helper functions
such as uinput_ff_request_end() to safely encapsulate the request completion
logic under the spinlock.

Link: https://lore.kernel.org/all/20240521113619.2092-1-hdanton@sina.com/

> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index d32fa4b508fc5..1a294f4e941b8 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c

[ ... ]

> +static int uinput_ff_request_end(struct uinput_device *udev,
> +				 unsigned int id, unsigned int code, int retval)
> +{
> +	struct uinput_request *req;
> +
> +	guard(spinlock)(&udev->requests_lock);
> +
> +	req = uinput_request_find(udev, id);
> +	if (!req || req->code != code)
> +		return -EINVAL;
> +
> +	if (code == UI_FF_UPLOAD && !req->u.upload.effect)
> +		return -EINVAL;
> +
> +	req->retval = retval;
> +	complete(&req->done);

[Severity: Low]
Does this code leave a data race on req->retval with the submitter?

Looking at uinput_request_submit(), the submitter reads request->retval
without holding requests_lock:

drivers/input/misc/uinput.c:uinput_request_submit() {
    ...
	if (!wait_for_completion_timeout(&request->done, 30 * HZ)) {
		retval = -ETIMEDOUT;
		goto out;
	}

	retval = request->retval;
    ...
}

If the completion is triggered by uinput_flush_requests() or if multiple
ioctls race to complete the same request, could the submitter wake up and
read request->retval outside of requests_lock concurrently with this write?

Should this use READ_ONCE()/WRITE_ONCE() or extend the lock to prevent
undefined behavior and KCSAN warnings?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013051553.71887-1-cenzhang@linux.microsoft.com?part=1