Re: [PATCH 1/2] input: atkbd: add softleds quirk for broken EC PS/2 emulation
From: sashiko-bot@kernel.org
Date: 2026-06-28 02:09:38
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [High] The `atkbd` driver mutates the parent `serio` port's `write` function pointer to `NULL`, which is a driver model violation and permanently corrupts the bus state. - [Medium] The newly added early return `if (atkbd->softleds) return 0;` inside `atkbd_set_leds()` is completely unreachable dead code. Pre-existing issues: - [High] A pre-existing race condition in `atkbd_disconnect()` can lead to a Use-After-Free of the `input_dev` structure in `atkbd_event_work()`. -- commit 449f71d6f3b7e8daf08e55bc0da16d0f59475df4 Author: Rodnei Cilto [off-list ref] input: atkbd: add softleds quirk for broken EC PS/2 emulation This commit introduces the atkbd_softleds mode to handle broken EC PS/2 emulation on some Lenovo IdeaPad laptops. It combines dumbkbd behaviour with EV_LED registration to keep LED states visible to userspace without sending the SETLEDS command. DMI entries are added for the Lenovo IdeaPad 83RR and 83SR.
quoted hunk ↗ jump to hunk
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c index 8cb4dc6fb1658..826a21dc016ab 100644 --- a/drivers/input/keyboard/atkbd.c +++ b/drivers/input/keyboard/atkbd.c@@ -600,6 +602,14 @@ static int atkbd_set_leds(struct atkbd *atkbd) struct input_dev *dev = atkbd->dev; u8 param[2]; + /* + * softleds: EC PS/2 emulation does not support AT commands + * after initialization. Accept LED state from userspace but + * never send SETLEDS (0xED) to avoid scancode corruption. + */ + if (atkbd->softleds) + return 0; +
[Severity: Medium] Is this early return reachable? When atkbd_softleds is active, atkbd_connect() sets atkbd->write = false. When an EV_LED event occurs, atkbd_event() checks atkbd->write and immediately returns -1, dropping the event before atkbd_schedule_event_work() is called. Does this mean atkbd_set_leds() is never executed by the workqueue in this mode, and this check will never be triggered? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1291,6 +1301,12 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv) if (atkbd->softrepeat) atkbd->softraw = true; + if (atkbd_softleds) { + serio->write = NULL; + atkbd->write = false; + atkbd->softleds = true; + } +
[Severity: High]
Does this permanently modify the parent bus provider's function pointer
(serio->write)?
If atkbd is unbound and another driver (e.g., serio_raw) binds to the same
port, or if atkbd is rebound, would the port remain crippled and unable to
transmit due to serio->write still being NULL?
[Severity: High]
This is a pre-existing issue, but can this sequence lead to a use-after-free
if a privileged user unbinds the driver while LED or repeat rate events are
being processed?
In atkbd_disconnect(), input_unregister_device() is called before
cancel_delayed_work_sync():
drivers/input/keyboard/atkbd.c:atkbd_disconnect() {
...
input_unregister_device(atkbd->dev);
/*
* Make sure we don't have a command in flight.
...
*/
cancel_delayed_work_sync(&atkbd->event_work);
...
}
If atkbd_event_work() starts executing on another CPU, it acquires
atkbd->mutex and checks atkbd->enabled. If preempted immediately after this
check, atkbd_disconnect() can proceed to set atkbd->enabled = false,
unregister and free the input device, and then block on
cancel_delayed_work_sync().
When the worker thread resumes, it calls atkbd_set_leds():
drivers/input/keyboard/atkbd.c:atkbd_event_work() {
...
if (!atkbd->enabled) {
...
} else {
if (test_and_clear_bit(ATKBD_LED_EVENT_BIT, &atkbd->event_mask))
atkbd_set_leds(atkbd);
...
}
Would atkbd_set_leds() then dereference the now-freed atkbd->dev?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-ideapad-kbd-leds-v1-0-50b421d94927@gmail.com?part=1