Re: [PATCH v5 2/2] HID: multitouch: fix use-after-free of haptic data on delayed input release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] input: module code UAF due to premature module_put() on unbind
- [High] hid: double-put of hdev and input_dev on multiple haptic touchpad collections
--

commit 058030eab60c1764e5ac8d06b32cafc8846c4171
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

HID: multitouch: fix use-after-free of haptic data on delayed input release

This commit moves haptic resource cleanup into a devm action so it runs at
driver unbind before the haptic struct is freed. It also pins the input
device and delays clearing ff->private to prevent double-frees if the
input device release is delayed.

> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> --- a/drivers/hid/hid-haptic.c
> +++ b/drivers/hid/hid-haptic.c
> @@ -379,32 +379,57 @@ static int hid_haptic_erase(struct input_dev *dev, int effect_id)
[ ... ]
> +static void hid_haptic_devm_release(void *data)
> +{
> +	struct hid_haptic_device *haptic = data;
> +	struct input_dev *dev = haptic->input_dev;
>  	int r;
>  
[ ... ]
> +	if (haptic->hdev)
> +		put_device(&haptic->hdev->dev);

[Severity: High]
If a composite HID device presents multiple HID_DG_TOUCHPAD application
collections with haptic usages, mt_input_configured() in hid-multitouch.c
will invoke hid_haptic_input_configured() and hid_haptic_init() multiple
times for the same device.

This causes hid_haptic_init() to queue multiple identical devm release
actions using the same haptic data pointer.

During unbind, this release function will execute multiple times. Because
haptic->hdev is not set to NULL after the first put_device(), will this
result in a double-put of the hdev reference count?

[ ... ]
> +	if (dev)
> +		input_put_device(dev);

[Severity: High]
Similarly, the local dev variable here is populated at the start of the
function from haptic->input_dev. If the release action runs multiple times
for the same haptic struct, will this result in a double-put of the
input_dev reference count as well?

> +
> +	module_put(THIS_MODULE);
>  }

[Severity: High]
The devres action runs synchronously during driver unbind. If an application
holds an open file descriptor to the input device, the input_dev structure
and its ff_device are kept alive after unbind.

Since module_put() executes here, the module's reference count drops, allowing
the system to unload the HID driver module.

When the application eventually closes the file descriptor, the last reference
to the input_dev is dropped. This triggers input_dev_release() which calls
input_ff_destroy().

input_ff_destroy() unconditionally invokes:

    if (ff->destroy)
        ff->destroy(ff);
        
Since dev->ff->destroy was not cleared and still points to hid_haptic_destroy()
inside the unloaded module, does this result in the execution of unmapped
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016135516.2957381-1-qwe.aldo@gmail.com?part=2