Re: [PATCH v13 3/7] input: serio: Add driver for ASUS Transformer dock keyboard and touchpad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] input: serio: notifier order in asus_ec_kbc_probe() causes NULL deref or UAF
--

commit 8f5c105d6df4708eb2c002e7fe58ff80a2a0fb84
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>

input: serio: Add driver for ASUS Transformer dock keyboard and touchpad

This commit adds a serio driver for the keyboard and touchpad on the ASUS
Transformer dock, registering two serio ports and a notifier on the parent
EC to forward and remap incoming PS/2 data.

> diff --git a/drivers/input/serio/asus-transformer-ec-kbc.c b/drivers/input/serio/asus-transformer-ec-kbc.c
> new file mode 100644
> index 0000000000000..cff18da6d59d7
> --- /dev/null
> +++ b/drivers/input/serio/asus-transformer-ec-kbc.c

[ ... ]

> +static int asus_ec_kbc_notify(struct notifier_block *nb,
> +			      unsigned long action, void *data_)
> +{

[ ... ]

> +	while (n--)
> +		serio_interrupt(priv->sdev[port_idx], *data++, 0);
> +
> +	return NOTIFY_OK;
> +}

[ ... ]

> +static int asus_ec_register_serio(struct platform_device *pdev, int idx, const char *name, int cmd)
> +{
> +	struct asus_ec_kbc_data *priv = platform_get_drvdata(pdev);
> +	struct i2c_client *parent = priv->parent;
> +	struct serio *port = kzalloc_obj(*port);
> +
> +	if (!port)
> +		return -ENOMEM;
> +
> +	priv->sdev[idx] = port;
> +	port->dev.parent = &pdev->dev;
> +	port->id.type = SERIO_8042;
> +	port->id.extra = cmd & 0xFF;
> +	port->write = asus_ec_serio_write;
> +	port->port_data = (void *)priv;
> +	snprintf(port->name, sizeof(port->name), "%s %s", priv->ec->model, name);
> +	snprintf(port->phys, sizeof(port->phys), "i2c-%u-%04x/serio%d",
> +		 i2c_adapter_id(parent->adapter), parent->addr, idx);
> +
> +	serio_register_port(port);
> +
> +	return devm_add_action_or_reset(&pdev->dev, asus_ec_serio_remove, port);
> +}
> +
> +static void asus_ec_notifier_chain_unregister(void *data)
> +{
> +	struct asus_ec_kbc_data *priv = data;
> +	struct asusec_core *ec = priv->ec;
> +
> +	blocking_notifier_chain_unregister(&ec->notify_list, &priv->nb);
> +}
> +
> +static int asus_ec_kbc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct asusec_core *ec = dev_get_drvdata(dev->parent);
> +	struct asus_ec_kbc_data *priv;
> +	int error;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->ec = ec;
> +	priv->parent = to_i2c_client(dev->parent);
> +
> +	priv->nb.notifier_call = asus_ec_kbc_notify;
> +	error = blocking_notifier_chain_register(&ec->notify_list, &priv->nb);
> +	if (error)
> +		return dev_err_probe(dev, error, "Failed to register blocking notifier chain");
> +
> +	error = devm_add_action_or_reset(dev, asus_ec_notifier_chain_unregister, priv);
> +	if (error)
> +		return error;
> +
> +	error = asus_ec_register_serio(pdev, 0, "Keyboard", 0);
> +	if (error)
> +		return error;
> +
> +	error = asus_ec_register_serio(pdev, 1, "Touchpad", I8042_CMD_AUX_SEND);
> +	if (error)
> +		return error;

[Severity: High]
As reported by Dmitry Torokhov on v12 [1], registering the notifier before
initializing the serio ports and without checking whether the ports are ready
can lead to a null pointer dereference during probe or a use-after-free
during teardown.

In the parent MFD driver, asus_ec_probe() enables the threaded IRQ handler
asus_ec_interrupt() before adding the child MFD devices. Once
asus_ec_kbc_probe() calls blocking_notifier_chain_register(), an incoming EC
keyboard or touchpad interrupt can invoke asus_ec_kbc_notify() while
priv->sdev[0] and priv->sdev[1] are still NULL (or after priv->sdev[idx] =
port in asus_ec_register_serio(), before serio_register_port() initializes
port->lock). Passing priv->sdev[port_idx] to serio_interrupt() will then
dereference NULL or lock an uninitialized spinlock.

Additionally, because devm_add_action_or_reset() for
asus_ec_notifier_chain_unregister() is registered before the
asus_ec_serio_remove() actions in asus_ec_register_serio(), devm teardown runs
in reverse order:

  asus_ec_serio_remove()
    serio_unregister_port()
      serio_disconnect_port()
      serio_destroy_port() -> frees struct serio
  ...
  asus_ec_notifier_chain_unregister()

During serio_disconnect_port(), the attached PS/2 driver sends disable/reset
commands over I2C, causing the EC to generate ACK interrupts while priv->nb
is still registered and priv->sdev[] holds dangling pointers to the freed
serio ports. Can the notifier be registered after the serio ports (or gated
on port readiness under a lock via serio start/stop or open/close callbacks)
so that interrupts cannot reach serio_interrupt() before registration or
after unregistration?

[1] https://lore.kernel.org/r/ate5K6ZdrSoBUI08@google.com

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