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

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

[Cen Zhang (Microsoft)]

uinput_request_find() reads udev->requests[id] without holding
requests_lock, and the UI_BEGIN_FF_* / UI_END_FF_* ioctl handlers then
keep using the pointer it returned: the BEGIN handlers copy data out of
the request (including *req->u.upload.effect, which points into the
evdev caller's stack), the END handlers set req->retval and call
complete(&req->done).

The problem is that the request is a local variable of the task
sleeping in uinput_request_submit(). Once that task wakes up it clears
the slot and returns, and the request is gone. It can wake up at any
time as far as the ioctl handler is concerned: another UI_END_FF_* may
have completed it already, uinput_flush_requests() may have completed
it because the device is being destroyed, or the 30 second timeout may
have expired. If that happens after the lookup, the handler ends up
reading from and writing to a stack frame that no longer exists, and
complete() then tries to take a spinlock that contains whatever the
stack was reused for.

An easy-to-hit case: two processes sharing a uinput fd and both issuing
UI_END_FF_UPLOAD for the same request id crash a KASAN kernel within
seconds.

  Oops: general protection fault, probably for non-canonical address
    0xdffffc0000000001: 0000 [#1] SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
  RIP: 0010:complete+0x95/0x260
  Call Trace:
   uinput_ioctl_handler.isra.0+0x4b7/0x1a40
   __x64_sys_ioctl+0x184/0x1d0

Fix by holding requests_lock from the lookup until we are done with
the request, similar to what uinput_flush_requests() already does.
uinput_request_release_slot() takes that lock too, so the submitter
can not release the slot (and return) while a handler is still using
the request. Once the slot is released, a lookup can no longer return
the expired request.

To keep copy_to_user()/copy_from_user() out of the locked region, the
part of each handler that touches the request is moved into a small
helper that runs under the lock.

Fixes: 6b4877c7bdc6 ("Input: uinput - avoid crash when sending FF request to device going away")
Fixes: 8e009118a45a ("Input: uinput - allow FF requests to time out")
Cc: stable@vger.kernel.org
Cc: Francis Perron <francis@akrites.dev>
Reported-by: Hillf Danton <hdanton@sina.com>
Closes: https://lore.kernel.org/all/20240521113619.2092-1-hdanton@sina.com/
Reported-by: AutonomousCodeSecurity@microsoft.com
Assisted-by: LLM
Signed-off-by: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>
---
 drivers/input/misc/uinput.c | 110 ++++++++++++++++++++++++------------
 1 file changed, 73 insertions(+), 37 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index d32fa4b508fc..1a294f4e941b 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -119,16 +119,80 @@ static bool uinput_request_alloc_id(struct uinput_device *udev,
 	return reserved;
 }
 
+/*
+ * Find an input request, by ID. Returns NULL if the ID isn't valid.
+ * Requests live on their submitter's stack, so the caller must hold
+ * requests_lock while using the result.
+ */
 static struct uinput_request *uinput_request_find(struct uinput_device *udev,
 						  unsigned int id)
 {
-	/* Find an input request, by ID. Returns NULL if the ID isn't valid. */
+	lockdep_assert_held(&udev->requests_lock);
+
 	if (id >= UINPUT_NUM_REQUESTS)
 		return NULL;
 
 	return udev->requests[id];
 }
 
+static int uinput_ff_upload_begin(struct uinput_device *udev,
+				  struct uinput_ff_upload *ff_up)
+{
+	struct uinput_request *req;
+
+	guard(spinlock)(&udev->requests_lock);
+
+	req = uinput_request_find(udev, ff_up->request_id);
+	if (!req || req->code != UI_FF_UPLOAD || !req->u.upload.effect)
+		return -EINVAL;
+
+	ff_up->retval = 0;
+	ff_up->effect = *req->u.upload.effect;
+	if (req->u.upload.old)
+		ff_up->old = *req->u.upload.old;
+	else
+		memset(&ff_up->old, 0, sizeof(ff_up->old));
+
+	return 0;
+}
+
+static int uinput_ff_erase_begin(struct uinput_device *udev,
+				 struct uinput_ff_erase *ff_erase)
+{
+	struct uinput_request *req;
+
+	guard(spinlock)(&udev->requests_lock);
+
+	req = uinput_request_find(udev, ff_erase->request_id);
+	if (!req || req->code != UI_FF_ERASE)
+		return -EINVAL;
+
+	ff_erase->retval = 0;
+	ff_erase->effect_id = req->u.effect_id;
+
+	return 0;
+}
+
+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);
+
+	return 0;
+}
+
 static int uinput_request_reserve_slot(struct uinput_device *udev,
 				       struct uinput_request *request)
 {
@@ -913,7 +977,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	struct uinput_device	*udev = file->private_data;
 	struct uinput_ff_upload ff_up;
 	struct uinput_ff_erase  ff_erase;
-	struct uinput_request   *req;
 	char			*phys;
 	const char		*name;
 	unsigned int		size;
@@ -1011,19 +1074,9 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 		if (retval)
 			goto out;
 
-		req = uinput_request_find(udev, ff_up.request_id);
-		if (!req || req->code != UI_FF_UPLOAD ||
-		    !req->u.upload.effect) {
-			retval = -EINVAL;
+		retval = uinput_ff_upload_begin(udev, &ff_up);
+		if (retval)
 			goto out;
-		}
-
-		ff_up.retval = 0;
-		ff_up.effect = *req->u.upload.effect;
-		if (req->u.upload.old)
-			ff_up.old = *req->u.upload.old;
-		else
-			memset(&ff_up.old, 0, sizeof(struct ff_effect));
 
 		retval = uinput_ff_upload_to_user(p, &ff_up);
 		goto out;
@@ -1034,14 +1087,10 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			goto out;
 		}
 
-		req = uinput_request_find(udev, ff_erase.request_id);
-		if (!req || req->code != UI_FF_ERASE) {
-			retval = -EINVAL;
+		retval = uinput_ff_erase_begin(udev, &ff_erase);
+		if (retval)
 			goto out;
-		}
 
-		ff_erase.retval = 0;
-		ff_erase.effect_id = req->u.effect_id;
 		if (copy_to_user(p, &ff_erase, sizeof(ff_erase))) {
 			retval = -EFAULT;
 			goto out;
@@ -1054,15 +1103,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 		if (retval)
 			goto out;
 
-		req = uinput_request_find(udev, ff_up.request_id);
-		if (!req || req->code != UI_FF_UPLOAD ||
-		    !req->u.upload.effect) {
-			retval = -EINVAL;
-			goto out;
-		}
-
-		req->retval = ff_up.retval;
-		complete(&req->done);
+		retval = uinput_ff_request_end(udev, ff_up.request_id,
+					       UI_FF_UPLOAD, ff_up.retval);
 		goto out;
 
 	case UI_END_FF_ERASE:
@@ -1071,14 +1113,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			goto out;
 		}
 
-		req = uinput_request_find(udev, ff_erase.request_id);
-		if (!req || req->code != UI_FF_ERASE) {
-			retval = -EINVAL;
-			goto out;
-		}
-
-		req->retval = ff_erase.retval;
-		complete(&req->done);
+		retval = uinput_ff_request_end(udev, ff_erase.request_id,
+					       UI_FF_ERASE, ff_erase.retval);
 		goto out;
 	}
 
-- 
2.55.0

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

[sashiko-bot]

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