[PATCH] HID: logitech-hidpp: cancel reset_hi_res_work on probe failure

[PATCH] HID: logitech-hidpp: cancel reset_hi_res_work on probe failure

[Dmitriy Geels]

The probe error path cancels hidpp->work before freeing the HID++ device
data, but does not cancel reset_hi_res_work.

reset_hi_res_work can be scheduled while probing the device. If probe
subsequently fails, the work can run after the driver data has been
freed.

Cancel reset_hi_res_work in the probe error path to prevent a potential
use-after-free.

Fixes: ed80cc4667ac ("HID: logitech-hidpp: Add HIDPP_QUIRK_RESET_HI_RES_SCROLL")

Signed-off-by: Dmitriy Geels <dmitriy.geels@gmail.com>
---
This patch is a result of Sashiko AI review for "HID: logitech-hidpp:
add native touchpad support for K400 Plus":
 - [High] Missing cancel_work_sync for reset_hi_res_work in the probe error path leads to a Use-After-Free.

 drivers/hid/hid-logitech-hidpp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 9a6b04283130..279384fa9835 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4925,6 +4925,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
+	cancel_work_sync(&hidpp->reset_hi_res_work);
 	mutex_destroy(&hidpp->send_mutex);
 	return ret;
 }
-- 
2.55.0

Re: [PATCH] HID: logitech-hidpp: cancel reset_hi_res_work on probe failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free in force feedback destruction callback and work handler
- [High] Sleeping in atomic context due to GFP_KERNEL allocation in force feedback path
--

--- Patch [1]: [PATCH] HID: logitech-hidpp: cancel reset_hi_res_work on probe failure ---
commit deab4716533d9354cbeb6d6d049fa4f6aafc98ce
Author: Dmitriy Geels <dmitriy.geels@gmail.com>

HID: logitech-hidpp: cancel reset_hi_res_work on probe failure

The probe error path cancels hidpp->work before freeing the HID++ device
data, but does not cancel reset_hi_res_work. Cancel reset_hi_res_work in
the probe error path to prevent a potential use-after-free.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1504de32b1c84..6a21b281a1df5 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c

[ ... ]

> @@ -4834,6 +4834,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  hid_hw_start_fail:
>  	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>  	cancel_work_sync(&hidpp->work);
> +	cancel_work_sync(&hidpp->reset_hi_res_work);
>  	mutex_destroy(&hidpp->send_mutex);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential Use-After-Free in the
force feedback teardown path.

If userspace holds an open file descriptor to the evdev node during unbind,
the input_dev object is kept alive after devres frees the hidpp private data.

When userspace eventually closes the file descriptor, input_free_device()
executes hidpp_ff_destroy():

drivers/hid/hid-logitech-hidpp.c:hidpp_ff_destroy() {
    ...
    struct hid_device *hid = data->hidpp->hid_dev;
    hid_info(hid, "Unloading HID++ force feedback.\n");
    device_remove_file(&hid->dev, &dev_attr_range);
    ...
}

Because data->hidpp has already been freed by devres, hidpp_ff_destroy()
dereferences a dangling pointer.

A similar UAF can occur in hidpp_ff_work_handler() if any force feedback work
runs asynchronously after unbind:

drivers/hid/hid-logitech-hidpp.c:hidpp_ff_work_handler() {
    ...
    ret = hidpp_send_fap_command_sync(data->hidpp, ...);
    ...
}

Could a local user trigger this by holding the device node open while the
device is unplugged? Does the driver need to explicitly manage the lifecycle
of the hidpp private data to prevent this?


[Severity: High]
This is a pre-existing issue, but I also noticed a potential sleep-in-atomic
bug in the force feedback implementation.

The input subsystem routes force feedback events through input_ff_event(),
which is called with the dev->event_lock spinlock held and local interrupts
disabled. This invokes the driver's FF callbacks:

drivers/hid/hid-logitech-hidpp.c:hidpp_ff_playback() {
    ...
    return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
}

Inside hidpp_ff_queue_work(), there is a GFP_KERNEL allocation:

drivers/hid/hid-logitech-hidpp.c:hidpp_ff_queue_work() {
    ...
    struct hidpp_ff_work_data *wd = kzalloc_obj(*wd);
    ...
}

Since kzalloc_obj expands to kzalloc with GFP_KERNEL, this performs a sleeping
allocation while holding a spinlock. 

Could a local user trigger a deadlock by sending EV_FF events to the evdev
node? Should this allocation use GFP_ATOMIC instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824173718.14687-1-dmitriy.geels@gmail.com?part=1