Re: [PATCH] HID: logitech-hidpp: Fix FF device cleanup on init failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Calling `input_ff_destroy(dev)` leaves a dangling `dev->flush` pointer, causing a guaranteed NULL pointer dereference when the device file is closed.
- [High] Calling `input_ff_destroy(dev)` on an active, registered device in the error path introduces a Use-After-Free race condition.

Pre-existing issues:
- [Critical] Use-After-Free in `hidpp_ff_destroy()` due to mismatched lifecycles between `devm`-managed driver data and `input_dev` lifecycle.
- [High] Input device capabilities are initialized after registration, violating subsystem lifecycle rules and exposing incomplete state to userspace.
--

commit 1a99b024f97c7dab9fe8188d726db401b73dd317
Author: Haoxiang Li <haoxiang_li2024@163.com>

    HID: logitech-hidpp: Fix FF device cleanup on init failure

    hidpp_ff_init() creates the input force-feedback device with
    input_ff_create(), then allocates the HID++ FF private data,
    effect ID array, and workqueue.
    
    If any of those allocations fail after input_ff_create() succeeds,
    the function returns an error without destroying the FF device.
    Add an unwind path that frees the private allocations made by
    hidpp_ff_init() and calls input_ff_destroy() for failures after
    input_ff_create() succeeds.

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ccbf28869a968..95f84efa77069 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2859,18 +2859,19 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 	 * ownership to FF core
 	 */

[Severity: High]
This is a pre-existing issue, but does initializing input device capabilities
after registration violate subsystem lifecycle rules and expose incomplete
state to userspace?

Because hidpp_probe() calls hid_connect() (which registers the device)
before calling hidpp_ff_init(), can userspace open the device and issue
ioctls immediately upon registration, before hidpp_ff_init() has fully
configured the FF capabilities:

    for (j = 0; hidpp_ff_effects[j] >= 0; j++)
        set_bit(hidpp_ff_effects[j], dev->ffbit);

>  	data = kmemdup(data, sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> +	if (!data) {
> +		error = -ENOMEM;
> +		goto err_destroy_ff;
> +	}

[ ... ]

>  	data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
>  	if (!data->wq) {
> -		kfree(data->effect_ids);
> -		kfree(data);
> -		return -ENOMEM;
> +		error = -ENOMEM;
> +		goto err_free_effect_ids;
>  	}
>  
>  	data->hidpp = hidpp;

[Severity: Critical]
This is a pre-existing issue, but could this lead to a use-after-free in
hidpp_ff_destroy() due to mismatched lifecycles?

The hidpp_device structure is allocated using devm_kzalloc() in hidpp_probe(),
meaning it is freed by devres immediately after the driver's remove()
callback completes.

However, the input_dev is reference-counted and outlives the driver binding
if a userspace process holds the event file descriptor open. When the process
finally closes the fd, input_free_device() calls hidpp_ff_destroy(), which
dereferences data->hidpp to access hid_dev:

    struct hid_device *hid = data->hidpp->hid_dev;

Could this trigger a use-after-free if a local user holds the input device
open during a device unplug or driver unbind?

[ ... ]

> @@ -2900,6 +2901,14 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
>  		 version);
>  
>  	return 0;
> +
> +err_free_effect_ids:
> +	kfree(data->effect_ids);
> +err_free_data:
> +	kfree(data);
> +err_destroy_ff:
> +	input_ff_destroy(dev);

[Severity: High]
Is it possible that calling input_ff_destroy(dev) here leaves a dangling
dev->flush pointer?

This frees dev->ff but does not clear the dev->flush = input_ff_flush
callback set earlier by input_ff_create().

Since hidpp_probe() ignores the initialization error, the input device
remains registered. If a userspace process opens and then closes the event
node, evdev_release() calls input_flush_device(), triggering
input_ff_flush():

    guard(mutex)(&ff->mutex);

Does this cause a guaranteed NULL pointer dereference because dev->ff is now
NULL?

[Severity: High]
Additionally, since hidpp_ff_init() runs after hid_connect() has already
registered the device, it is visible to userspace.

If input_ff_create() succeeds, it sets the EV_FF bit. If a concurrent thread
immediately issues an EVIOCSFF ioctl, calling input_ff_upload() which uses
dev->ff, and this error path triggers simultaneously calling
input_ff_destroy(dev):

    input_ff_destroy(dev);

Could this regression result in a use-after-free in the force-feedback ioctl
handler without any synchronization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623065200.1966629-1-haoxiang_li2024@163.com?part=1