Re: [PATCH] Input: evdev: drain queued events before reporting device removal

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

New issues:
- [High] The patch breaks zero-length O_NONBLOCK reads by returning 0 instead of -EAGAIN when the queue is empty, altering userspace ABI.
- [High] A TOCTOU race between lockless queue checks and device death drops synthetic key-up events, defeating the patch's purpose.

Pre-existing issues:
- [High] Uninitialized kernel stack memory is leaked to user-space due to missing padding initialization in `struct input_event` on architectures like SPARC64.
--

--- Patch [1]: [PATCH] Input: evdev: drain queued events before reporting device removal ---
commit 0c2e6f64ca8c023851919612ec0b8e230776b440
Author: zhangliuyang.zly <[email protected]>
Subject: Input: evdev: drain queued events before reporting device removal

This commit modifies the evdev read path to drain the per-client event queue
before reporting -ENODEV when a device is removed. It aims to prevent dropping
synthetic key-up events generated during device unregistration.

> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c

[ ... ]

> @@ -565,20 +565,38 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
>  	if (count != 0 && count < input_event_size())
>  		return -EINVAL;
>  
> -	for (;;) {
> +	/*
> +	 * count == 0 is special - no IO is done but we still check for
> +	 * error conditions, preserving the historical behavior.
> +	 */
> +	if (count == 0) {
> +		if (!evdev->exist || client->revoked)
> +			return -ENODEV;
> +
> +		return 0;
> +	}

[Severity: High]
Does this change alter the historical behavior for non-blocking reads?

Previously, if count == 0 and the file descriptor was set to O_NONBLOCK with
an empty queue, the code would return -EAGAIN. By moving the count == 0 check
above the loop and unconditionally returning 0, applications checking for queue
status using a zero-length read might now incorrectly assume events are
available.

>  
> +	for (;;) {
> +		/*
> +		 * A revoked client must not consume any more events.
> +		 */
> +		if (client->revoked)
> +			return -ENODEV;
> +
> +		/*
> +		 * The input core may queue synthetic release events during
> +		 * device unregister before evdev_disconnect() marks the evdev
> +		 * node dead. Do not drop those already queued events by
> +		 * returning -ENODEV too early. Drain the client queue first and
> +		 * report -ENODEV only when there is nothing left to read.
> +		 */
> +		if (client->packet_head == client->tail) {
> +			if (!evdev->exist)
> +				return -ENODEV;

[Severity: High]
Might this lockless conditional block still drop synthetic key-up events due
to a race condition?

Because these checks are performed locklessly without memory barriers, a thread
could evaluate packet_head == tail as true. If a context switch occurs and the
input core queues synthetic events and subsequently marks evdev->exist as false,
when this thread resumes it will evaluate !evdev->exist as true.

This would result in returning -ENODEV and missing the newly queued synthetic
events entirely, defeating the original purpose of the patch.

> +
> +			if (file->f_flags & O_NONBLOCK)
> +				return -EAGAIN;
> +		}
>  
>  		while (read + input_event_size() <= count &&
>  		       evdev_fetch_next_event(client, &event)) {
>  
> -			/*
> -			 * count == 0 is special - no IO is done but we check
> -			 * for error conditions (see above).
> -			 */
> -			if (count == 0)
> -				break;
> +			if (input_event_to_user(buffer + read, &event))
> +				return -EFAULT;

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

In evdev_pass_values(), the input_event structure is allocated on the stack
but its padding fields (such as the 32-bit padding on SPARC64) are not fully
initialized before being copied into the client buffer.

When evdev_read() fetches these events via evdev_fetch_next_event() and
subsequently copies them to userspace using input_event_to_user():

evdev_pass_values() {
    struct input_event event;
    ...
    event.type = v->type;
    ...
    __pass_event(client, &event);
}

The uninitialized padding bytes are copied out here, potentially leaking
sensitive kernel data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1