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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+static int asus_ec_kbc_notify(struct notifier_block *nb,
+ unsigned long action, void *data_)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ while (n--)
+ serio_interrupt(priv->sdev[port_idx], *data++, 0);
+
+ return NOTIFY_OK;
+}
[ ... ]
quoted hunk ↗ jump to hunk
+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 (local)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261104122606.215629-1-clamor95@gmail.com?part=3