[PATCH] HID: haptic: fix use-after-free of devm haptic data in hid_haptic_destroy()

[PATCH] HID: haptic: fix use-after-free of devm haptic data in hid_haptic_destroy()

[Aldo Ariel Panzardo]

mt_probe() allocates td->haptic with devm_kzalloc(), tying its
lifetime to the HID device's driver unbind.  hid_haptic_init() then
installs hid_haptic_destroy() as the force-feedback destroy callback,
which dereferences haptic->hdev on its very first line.

When the HID device is removed while a process still holds an evdev
fd, devres frees td->haptic at unbind time, but hid_haptic_destroy()
runs later from input_dev_release() when the last fd closes.  The
callback operates on freed memory.

The existing get_device()/put_device() pair in init/destroy pins the
struct hid_device but does not keep its devres allocations alive,
since devres runs at driver unbind, not at the final device kref put.

Replace devm_kzalloc() with plain kzalloc() for the haptic struct so
it outlives the driver.  Free it at the end of hid_haptic_destroy(),
which already tears down every sub-allocation manually and holds a
device reference that keeps hdev alive until the kfree.  On the
non-haptic path and the error paths in mt_probe(), use kfree()
instead of devm_kfree().

Fixes: 8d0bf7908b5a ("HID: multitouch: add haptic multitouch support")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/hid/hid-haptic.c     |  2 ++
 drivers/hid/hid-multitouch.c | 18 ++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index 8760eeb08..6c365dbf0 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -406,6 +406,8 @@ static void hid_haptic_destroy(struct ff_device *ff)
 	haptic->hid_usage_map = NULL;
 
 	module_put(THIS_MODULE);
+
+	kfree(haptic);
 }
 
 int hid_haptic_init(struct hid_device *hdev,
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 4e19a0c4d..4f7b97cd4 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2132,7 +2132,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
 		return -ENOMEM;
 	}
-	td->haptic = devm_kzalloc(&hdev->dev, sizeof(*(td->haptic)), GFP_KERNEL);
+	td->haptic = kzalloc(sizeof(*(td->haptic)), GFP_KERNEL);
 	if (!td->haptic)
 		return -ENOMEM;
 
@@ -2181,12 +2181,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ret = hid_parse(hdev);
 	if (ret != 0)
-		return ret;
+		goto err_free_haptic;
 
 	if (mtclass->name == MT_CLS_APPLE_TOUCHBAR &&
 	    !hid_find_field(hdev, HID_INPUT_REPORT,
-			    HID_DG_TOUCHPAD, HID_DG_TRANSDUCER_INDEX))
-		return -ENODEV;
+			    HID_DG_TOUCHPAD, HID_DG_TRANSDUCER_INDEX)) {
+		ret = -ENODEV;
+		goto err_free_haptic;
+	}
 
 	if (mtclass->quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)
 		mt_fix_const_fields(hdev, HID_DG_CONTACTID);
@@ -2196,7 +2198,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
-		return ret;
+		goto err_free_haptic;
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
 	if (ret)
@@ -2206,9 +2208,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
 
 	if (!td->is_haptic_touchpad)
-		devm_kfree(&hdev->dev, td->haptic);
+		kfree(td->haptic);
 
 	return 0;
+
+err_free_haptic:
+	kfree(td->haptic);
+	return ret;
 }
 
 static int mt_suspend(struct hid_device *hdev, pm_message_t state)
-- 
2.43.0

Re: [PATCH] HID: haptic: fix use-after-free of devm haptic data in hid_haptic_destroy()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] input: guaranteed double free of haptic structure in hid_haptic_destroy()
- [High] hid: double free of td->haptic on mt_probe() error paths
- [Medium] hid: memory leak of td->haptic for non-touchpad devices with pressure
--

commit 9428ea91bd775df855a2be1ddad3deee003770fa
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

HID: haptic: fix use-after-free of devm haptic data in hid_haptic_destroy()

This commit changes td->haptic allocation from devm_kzalloc() to kzalloc() 
to prevent a use-after-free when the HID device is unbound while evdev is 
still open. It also adds explicit kfree() calls in the destruction callback
and error paths to manage the memory manually.

> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> index 8760eeb08b2cd..6c365dbf0b9cf 100644
> --- a/drivers/hid/hid-haptic.c
> +++ b/drivers/hid/hid-haptic.c
[ ... ]
> @@ -406,6 +406,8 @@ static void hid_haptic_destroy(struct ff_device *ff)
>  	haptic->hid_usage_map = NULL;
>  
>  	module_put(THIS_MODULE);
> +
> +	kfree(haptic);

[Severity: High]
Does this code introduce a double free?

This function is registered as the ff->destroy callback for the
force-feedback device in hid_haptic_init(). The input core's
input_ff_destroy() function unconditionally calls kfree(ff->private)
immediately after executing the ff->destroy callback.

Since haptic and ff->private point to the exact same memory, it looks
like this will result in a double-free whenever the force-feedback device
is destroyed.

>  }
>  
>  int hid_haptic_init(struct hid_device *hdev,
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 4e19a0c4d2381..4f7b97cd4baa5 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
[ ... ]
> @@ -2196,7 +2198,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret)
> -		return ret;
> +		goto err_free_haptic;
>  
>  	ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
>  	if (ret)
> @@ -2206,9 +2208,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
>  
>  	if (!td->is_haptic_touchpad)
> -		devm_kfree(&hdev->dev, td->haptic);
> +		kfree(td->haptic);

[Severity: Medium]
Will this leak td->haptic for non-touchpad devices reporting pressure?

If the device reports pressure, mt_input_mapping() sets 
td->is_haptic_touchpad = true. Then in mt_input_configured(), if the
device application is not a touchpad (e.g. a touchscreen), 
hid_haptic_input_configured() immediately returns -1.

Because -1 is not 0, mt_input_configured() fails to reset 
td->is_haptic_touchpad to false.

The haptic setup is aborted, so the memory is never attached to the input
core. Later, mt_probe() sees td->is_haptic_touchpad as true and bypasses
this kfree(), leaking the memory since it is no longer devm-managed.

>  
>  	return 0;
> +
> +err_free_haptic:
> +	kfree(td->haptic);

[Severity: High]
Can this kfree() result in a double-free during partial initialization
failures?

If hid_hw_start() successfully sets up the haptic device via 
hid_haptic_init(), ownership of td->haptic is transferred to the input core 
as ff->private.

If a subsequent initialization step inside hid_hw_start() fails (like 
registering a secondary input interface), the HID core unwinds by 
destroying registered input devices. This calls input_ff_destroy() and 
frees ff->private.

When hid_hw_start() returns an error code, mt_probe() jumps here to 
err_free_haptic and explicitly executes kfree(td->haptic), freeing the
same memory again.

> +	return ret;
>  }
>  
>  static int mt_suspend(struct hid_device *hdev, pm_message_t state)

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