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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect registration order of the notifier and serio ports leads to NULL pointer dereferences during probe and use-after-free bugs during driver removal.
--

--- Patch [3]: [PATCH v10 3/7] input: serio: Add driver for ASUS Transformer dock keyboard and touchpad ---
commit 4e9b70c95d0f43e6f5bf78aeff2f78f72b9b2f82
Author: Michał Mirosław <[email protected]>

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

> 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..c6d12c44449d0
> --- /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_kbc_probe(struct platform_device *pdev)
> +{
> +	struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct asus_ec_kbc_data *priv;
> +	int error;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->ec = ec;
> +	priv->parent = to_i2c_client(pdev->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(&pdev->dev, error,
> +				     "failed to register blocking notifier chain");
> +
> +	error = devm_add_action_or_reset(&pdev->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);

[Severity: High]
Does this registration order leave the driver vulnerable to race conditions?

Since the parent MFD device might have already enabled its IRQ, an interrupt
can fire immediately after blocking_notifier_chain_register() completes. If
this occurs before the serio ports are registered, asus_ec_kbc_notify() will
read priv->sdev[port_idx] which is still NULL, and pass it to
serio_interrupt(), leading to a NULL pointer dereference.

Additionally, because devm actions execute in LIFO order during driver
removal or probe failure, the serio ports will be unregistered and freed
before the notifier is unregistered. If an interrupt fires during this
teardown window, could it pass a freed serio pointer to serio_interrupt()
and cause a use-after-free?

Should the notifier be registered only after the serio ports are fully
allocated and registered to prevent these crashes?

> +	if (error)
> +		return error;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3