Re: [PATCH] Input: atkbd - factor out atkbd_recreate_device() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The error recovery path in atkbd_recreate_device() corrupts the active input device by clearing its EV_SYN capability, rendering the keyboard completely unresponsive.
--

--- Patch [1]: [PATCH] Input: atkbd - factor out atkbd_recreate_device() helper ---
commit 90c8b25166b08db625d55de3789ce58cd2cfc65c
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: atkbd - factor out atkbd_recreate_device() helper

This commit factors out the common logic for unregistering the old input
device, allocating a new one, updating state, and registering the new device
into a new atkbd_recreate_device() helper to reduce code duplication.

> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 8cb4dc6fb1658..cc061bd0761f4 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -1452,6 +1452,47 @@ static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t

[ ... ]

> +static int atkbd_recreate_device(struct atkbd *atkbd,
> +				 unsigned char new_set, bool new_extra)
> +{
> +	struct input_dev *new_dev, *old_dev = atkbd->dev;
> +	bool old_extra = atkbd->extra;
> +	u8 old_set = atkbd->set;
> +	int err;

[ ... ]

> +	err = input_register_device(atkbd->dev);
> +	if (err) {
> +		input_free_device(new_dev);
> +
> +		atkbd->dev = old_dev;
> +		atkbd->set = atkbd_select_set(atkbd, old_set, old_extra);
> +		atkbd_set_keycode_table(atkbd);
> +		atkbd_set_device_attrs(atkbd);

[Severity: High]
This is a pre-existing issue, but does calling atkbd_set_device_attrs() on
the already-registered old_dev corrupt its capabilities during this error
recovery path?

Looking at atkbd_set_device_attrs(), it initializes capabilities using a
direct assignment:

    input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
        BIT_MASK(EV_MSC);

This unconditionally clears bit 0 (EV_SYN). For a newly allocated device
this is fine, as input_register_device() adds EV_SYN later.

However, when called on the active old_dev after input_register_device()
fails, it strips the EV_SYN capability. Without EV_SYN, does the input core
drop all synchronization events (SYN_REPORT), leaving key events stuck in
client buffers and rendering the keyboard unresponsive?

> +
> +		return err;
> +	}
> +
> +	input_unregister_device(old_dev);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/anQg6dtUwQ1Fx4hM@google.com?part=1