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

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

[Jeffin Philip]

Commit 0ef7a26af127 ("Input: atkbd - fix canceling event_work in disconnect")
moved cancel_delayed_work_sync() after input_unregister_device() on
the premise that events may arrive until input_unregister_device returns.

However, this created a UAF as work that may have passed the
atkbd->enabled check in atkbd_event_work() may attempt to dereference dev
which is freed in input_unregister_device(). Reverting the commit also
does not solve the issue as events may still come through and pass the
atkbd->enabled check as atkbd_disable() and atkbd_event_work() guard with different
locks. Fix this by closing the hardware first using serio_close()
and then unregistering to prevent work from executing after
input_unregister_device(). serio_close() closes the device preventing
work from arriving.

Reported-by: syzbot+1e2ef9bcb29af666b2e6@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1e2ef9bcb29af666b2e6
Fixes: 0ef7a26af127 ("Input: atkbd - fix canceling event_work in disconnect")
Signed-off-by: Jeffin Philip <jeffinphilip14@gmail.com>
---
 drivers/input/keyboard/atkbd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 5736f4bc5a50..8874d9007a67 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -963,17 +963,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);
+
 	serio_set_drvdata(serio, NULL);
 	kfree(atkbd);
 }
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Relocating cancel_delayed_work_sync() before input_unregister_device() leaves a race window where userspace can re-arm the delayed work, leading to a Use-After-Free of the atkbd structure.
--

commit 3288c579c8b7f19a0df10dc75d7e14b7f368e60b
Author: Jeffin Philip <jeffinphilip14@gmail.com>

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

Commit 0ef7a26af127 ("Input: atkbd - fix canceling event_work in disconnect")
moved cancel_delayed_work_sync() after input_unregister_device() on
the premise that events may arrive until input_unregister_device returns.

However, this created a UAF as work that may have passed the
atkbd->enabled check in atkbd_event_work() may attempt to dereference dev
which is freed in input_unregister_device(). Reverting the commit also
does not solve the issue as events may still come through and pass the
atkbd->enabled check as atkbd_disable() and atkbd_event_work() guard with different
locks. Fix this by closing the hardware first using serio_close()
and then unregistering to prevent work from executing after
input_unregister_device(). serio_close() closes the device preventing
work from arriving.

Reported-by: syzbot+1e2ef9bcb29af666b2e6@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1e2ef9bcb29af666b2e6
Fixes: 0ef7a26af127 ("Input: atkbd - fix canceling event_work in disconnect")

> Fix this by closing the hardware first using serio_close()
> and then unregistering to prevent work from executing after
> input_unregister_device(). serio_close() closes the device preventing
> work from arriving.

Is this description accurate?

While serio_close() stops data from the hardware port, it does not prevent
userspace events from arriving via the input core. Userspace can still trigger
atkbd_event() by writing to the input device node (e.g. EV_LED or EV_REP)
as long as the input device remains registered.

> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 3509b58e64920..096bfbe241db0 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -963,17 +963,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]
Does moving cancel_delayed_work_sync() before input_unregister_device() leave
a race window where userspace can re-arm the delayed work?

Because the input device is still registered when cancel_delayed_work_sync()
returns, a local user with write access to the input device node can
concurrently write to it. This invokes atkbd_event(), which schedules the
work unconditionally:

drivers/input/keyboard/atkbd.c:atkbd_event() {
    ...
	switch (type) {
	case EV_LED:
		atkbd_schedule_event_work(atkbd, ATKBD_LED_EVENT_BIT);
		return 0;
    ...
}

>  	serio_set_drvdata(serio, NULL);
>  	kfree(atkbd);

[Severity: High]
If userspace re-arms the delayed work during the race window, will this kfree()
lead to a use-after-free when the re-armed timer fires?

The re-armed work item could execute after the atkbd structure is freed, and
immediately dereference the freed pointer when taking the mutex:

drivers/input/keyboard/atkbd.c:atkbd_event_work() {
    ...
	guard(mutex)(&atkbd->mutex);
    ...
}

>  }

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