Re: [PATCH] Input: reject parent overrides on managed input devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] input: newly added parent check breaks existing managed drivers
- [High] input: NULL pointer dereference in input_free_device()
--

commit 5c214b0cacbe95369ec05dede9ca4ab2c7e279f1
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: reject parent overrides on managed input devices

This commit adds a strict check to ensure that the parent of a managed
input device is not overridden or cleared before registration. It returns
-EINVAL if the parent does not match the original device that allocated
the managed input device. KUnit tests are also added to verify this
behavior.

> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 78c10eea7328d..1cca6d48f5977 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -2396,6 +2396,14 @@ int input_register_device(struct input_dev *dev)
>  	}
>  
>  	if (dev->devres_managed) {
> +		if (!dev->dev.parent ||
> +		    !devres_find(dev->dev.parent, devm_input_device_release,
> +				 devm_input_device_match, dev)) {
> +			dev_err(&dev->dev,
> +				"Parent of managed input device was overridden, refusing to register\n");
> +			return -EINVAL;

[Severity: High]
Does this new strict check break existing in-tree drivers that legitimately
reparent the input device during probe?

For example, looking at da7280_probe() in drivers/input/misc/da7280.c:

da7280_probe() {
    input_dev = devm_input_allocate_device(dev);
    ...
    input_dev->dev.parent = client->dev.parent;
    ...
    error = input_register_device(input_dev);
}

Another example is tps6507x_ts_probe() in drivers/input/touchscreen/tps6507x-ts.c:

tps6507x_ts_probe() {
    poll_dev = devm_input_allocate_device(&i2c->dev);
    ...
    poll_dev->dev.parent = tsc->dev;
    ...
    error = input_register_device(poll_dev);
}

Because these drivers explicitly override dev.parent before registration,
wouldn't this new check unconditionally fail with -EINVAL and break their
initialization sequence?

[Severity: High]
If this error path is triggered, can it lead to a NULL pointer dereference
or double-free when the driver cleans up?

When a managed driver's probe fails here, it will typically call
input_free_device() in its error path.  However, input_free_device() trusts
dev->dev.parent unconditionally:

drivers/input/input.c:input_free_device() {
    ...
    if (dev->devres_managed)
        WARN_ON(devres_destroy(dev->dev.parent,
                               devm_input_device_release,
                               devm_input_device_match,
                               dev));
    ...
}

If dev.parent was cleared to NULL before input_register_device() was called,
wouldn't passing NULL to devres_destroy() cause a crash when it tries to
lock the devres list?

Alternatively, if the device was reparented to a different device,
devres_destroy() would fail to find the devres on the new parent.  Wouldn't
this fail to untrack the devres, leading to a double-free later when the
original devm owner is unbound?

> +		}
> +
>  		devres = devres_alloc(devm_input_device_unregister,
>  				      sizeof(*devres), GFP_KERNEL);
>  		if (!devres)

[ ... ]

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