Re: [PATCH v2] Input: atkbd - fix UAF in atkbd_set_repeat_rate() on disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `atkbd_event()` allows delayed work to be queued after `cancel_delayed_work_sync()`, leading to a Use-After-Free of the `atkbd` structure.
--

commit 0f8148a1f313524a8dd641967f56cd788e4c8ac4
Author: Jeffin Philip <jeffinphilip14@gmail.com>

Input: atkbd - fix UAF in atkbd_set_repeat_rate() on disconnect

This commit attempts to fix a use-after-free issue by closing the serio
port before canceling delayed work and adding an enabled check to
atkbd_event(). It aims to prevent work from being scheduled after
disconnection.

Link: https://lore.kernel.org/all/20260828080839.116065-1-jeffinphilip14@gmail.com/T/#u

> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -672,6 +672,9 @@ static int atkbd_event(struct input_dev *dev,
>  	if (!atkbd->write)
>  		return -1;
>  
> +	if (!atkbd->enabled)
> +		return -1;
> +

[Severity: High]
Can this lockless check introduce a Time-of-Check to Time-of-Use (TOCTOU) race
condition?

If userspace calls evdev_write(), which invokes atkbd_event(), a thread could
read atkbd->enabled as true here, but then stall or be preempted before
actually scheduling the work.

[ ... ]

> @@ -963,17 +966,18 @@ static void atkbd_disconnect(struct serio *serio)
>  
>  	atkbd_disable(atkbd);
>  
> -	input_unregister_device(atkbd->dev);
> -
>  	/*
> -	 * Make sure we don't have a command in flight.
> -	 * Note that since atkbd->enabled is false event work will keep
> -	 * rescheduling itself until it gets canceled and will not try
> -	 * accessing freed input device or serio port.
> +	 * close serio first so device will not get any data, which prevents
> +	 * atkbd_event_work from being rescheduled after cancel_delayed_work_sync
> +	 * returns. This ensures no work can dereference atkbd->dev after it has
> +	 * been freed.
>  	 */
> -	cancel_delayed_work_sync(&atkbd->event_work);
>  
>  	serio_close(serio);
> +	cancel_delayed_work_sync(&atkbd->event_work);
> +
> +	input_unregister_device(atkbd->dev);
> +

[Severity: High]
If the thread running atkbd_event() is delayed after checking atkbd->enabled,
could it resume here and queue work after cancel_delayed_work_sync() has
already returned?

Since serio_close() only stops hardware interrupts and does not block userspace
from invoking atkbd_event(), atkbd_disconnect() could proceed to unregister
the device and free the atkbd structure.

When the delayed thread finally resumes and queues the work, won't that queued
work execute later and dereference the freed atkbd structure?

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