Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver registers a global input handler to snoop keystrokes from all input devices in the system, violating driver boundaries.
- [High] The input_dev's name, phys, and keycode fields point to devres-managed memory that is freed when the driver unbinds, causing a Use-After-Free if userspace holds the input device open.
- [Medium] The state flags `special_key_pressed` and `special_key_mode` are accessed and modified concurrently from multiple contexts without any locking or synchronization.
- [Low] The driver unregisters the input handler before unregistering the hardware event notifier in the remove function, violating the reverse-order teardown principle.
--

commit ed5ee673506765c49f751d4f9cce6ec50eda686c
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Subject: input: keyboard: Add driver for ASUS Transformer dock multimedia keys

This commit adds support for the multimedia top button row of the
ASUS Transformer's Mobile Dock keyboard. It switches to multimedia keys
when ScreenLock + AltGr are pressed.

> diff --git a/drivers/input/keyboard/asus-transformer-ec-keys.c b/drivers/input/keyboard/asus-transformer-ec-keys.c
> new file mode 100644
> index 0000000000000..bf6db002a8537
> --- /dev/null
> +++ b/drivers/input/keyboard/asus-transformer-ec-keys.c
> @@ -0,0 +1,286 @@
[ ... ]
> +static void asus_ec_input_event(struct input_handle *handle,
> +				unsigned int event_type,
> +				unsigned int event_code, int value)
> +{
> +	struct asus_ec_keys_data *priv = handle->handler->private;
> +
> +	/* Store special key state */
> +	if (event_type == EV_KEY && event_code == KEY_RIGHTALT)
> +		priv->special_key_pressed = !!value;

[Severity: Medium]
Can this result in a data race on special_key_pressed?

The asus_ec_input_event() function writes to special_key_pressed from the
input event delivery context, which often runs in hardirq or softirq.

Concurrently, asus_ec_keys_process_key() reads this flag and modifies
special_key_mode from a process context via the blocking notifier chain.

Could concurrent execution lead to missed key modifiers or lost mode updates
without synchronization primitives or READ_ONCE() annotations?

> +}
[ ... ]
> +static const struct input_device_id asus_ec_input_ids[] = {
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> +		.evbit = { BIT_MASK(EV_KEY) },
> +	},
> +	{ }
> +};

[Severity: High]
Does this input_device_id match any device with EV_KEY capabilities, causing
the driver to intercept key events globally?

If a user unplugs an unrelated external USB keyboard while the RIGHTALT key
is currently held down, could the special_key_pressed state remain
permanently stuck?

[ ... ]
> +static int asus_ec_keys_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
[ ... ]
> +	priv->xidev = devm_input_allocate_device(dev);
> +	if (!priv->xidev)
> +		return -ENOMEM;
> +
> +	priv->xidev->name = devm_kasprintf(dev, GFP_KERNEL, "%s Keyboard Ext",
> +					   ec->model);
> +	priv->xidev->phys = devm_kasprintf(dev, GFP_KERNEL, "i2c-%u-%04x",
> +					   i2c_adapter_id(parent->adapter),
> +					   parent->addr);

[Severity: High]
Does this code introduce a use-after-free regression if userspace holds the
input device open during an unbind?

The priv->xidev->name and priv->xidev->phys strings, as well as the
priv->keymap array assigned to dev->keycode, are managed by devres and will
be freed when the driver unbinds.

However, if a userspace process holds a file descriptor to the evdev node,
the input_dev object is kept alive via reference counting.

Could a subsequent ioctl from userspace, such as EVIOCGNAME, attempt to
read from these already-freed pointers?

[ ... ]
> +static void asus_ec_keys_remove(struct platform_device *pdev)
> +{
> +	struct asus_ec_keys_data *priv = platform_get_drvdata(pdev);
> +	struct asusec_core *ec = priv->ec;
> +
> +	input_unregister_handler(&priv->input_handler);
> +	blocking_notifier_chain_unregister(&ec->notify_list, &priv->nb);

[Severity: Low]
Does this remove sequence violate the reverse-order teardown principle?

In asus_ec_keys_probe(), the driver registers the input handler first,
followed by the EC notifier chain.

Unregistering the handler first opens a race window where the software
tracking is disconnected but the driver is still registered to receive
hardware events from the EC.

Can the driver still process events in a partially dismantled state?

> +}

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