Re: [PATCH 1/2] input: atkbd: add softleds quirk for broken EC PS/2 emulation

[sashiko-bot@kernel.org]

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 <rodnei.cilto@gmail.com>

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.

> 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?

[ ... ]

> @@ -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