Re: [PATCH] Input: fix potential use-after-free in input_devices_seq_show

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] mm: reference count imbalance in input_devices_seq_*
- [Critical] mm: invalid pointer dereference of ERR_PTR in input_devices_seq_stop
- [Medium] locking: unnecessary device refcounting due to flawed concurrency premise
--

commit 0ec10a6d37b0ca4c2e2e4d84fe21437f9040399a
Author: Habil Eren Türker <habilerenturker@gmail.com>

Input: fix potential use-after-free in input_devices_seq_show

This patch attempts to fix a potential use-after-free by taking a reference
to the input device during sequence iteration. It acquires the reference in
input_devices_seq_start() and drops it in input_devices_seq_stop().

> The input_devices_seq_show() function accesses the input_dev structure
> while holding input_mutex. However, the device can still be freed
> concurrently, leading to a use-after-free.

[Severity: Medium]
Can we re-evaluate if this concurrent free is structurally possible?

Looking at __input_unregister_device(), the device is removed from
input_dev_list strictly inside a scoped_guard(mutex, &input_mutex) block.
Because seq_file iteration holds input_mutex for the duration of the loop,
unregistration and freeing cannot occur concurrently with sequence iteration.

The devices in the list appear to be inherently pinned by the mutex.

> diff --git a/drivers/input/input.c b/drivers/input/input.c
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1124,7 +1125,11 @@ static void *input_devices_seq_start(struct seq_file *seq, loff_t *pos)
>  
>  	state->mutex_acquired = true;
>  
> -	return seq_list_start(&input_dev_list, *pos);
> +	v = seq_list_start(&input_dev_list, *pos);
> +	if (v)
> +		input_get_device(container_of(v, struct input_dev, node));
> +
> +	return v;
>  }
>  
>  static void *input_devices_seq_next(struct seq_file *seq, void *v, loff_t *pos)
>  {
>  	return seq_list_next(v, &input_dev_list, pos);
>  }

[ ... ]

> @@ -1140,6 +1145,17 @@ static void input_seq_stop(struct seq_file *seq, void *v)
>  		mutex_unlock(&input_mutex);
>  }
>  
> +static void input_devices_seq_stop(struct seq_file *seq, void *v)
> +{
> +	struct input_seq_state *state = seq->private;
> +
> +	if (v)
> +		input_put_device(container_of(v, struct input_dev, node));

[Severity: Critical]
Does this introduce a reference count imbalance during iteration?

In input_devices_seq_start(), a reference is acquired for the first element.
However, input_devices_seq_next() advances the iterator without releasing the
previous element's reference or acquiring a new one for the next element.

When input_devices_seq_stop() is called, it drops a reference on whichever
element the iteration halted on. If iteration stops on any element other than
the first, the first element's reference is permanently leaked, and the final
element's reference count will underflow.

[Severity: Critical]
Is there a risk of an invalid pointer dereference if mutex_lock_interruptible()
fails?

If mutex_lock_interruptible(&input_mutex) in input_devices_seq_start() is
interrupted, it returns ERR_PTR(-EINTR). The seq_file core passes this error
pointer to input_devices_seq_stop() as 'v'.

Since an ERR_PTR is not NULL, the 'if (v)' check passes. Does this cause
container_of() and input_put_device() to dereference a garbage pointer? Should
this check be 'if (v && !IS_ERR(v))' instead?

> +
> +	if (state->mutex_acquired)
> +		mutex_unlock(&input_mutex);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928170051.58695-1-habilerenturker@hotmail.com?part=1