[PATCH] HID: universal-pidff: stop the device when force-feedback init fails

[PATCH] HID: universal-pidff: stop the device when force-feedback init fails

[Baul Lee]

universal_pidff_probe() starts the device with hid_hw_start() and then, if
force-feedback initialisation fails, returns the error through a label that
only does "return error".  The device is left started.

The HID core does not unwind on the driver's behalf.  __hid_device_probe()
releases the devres group, closes the report and clears hdev->driver:

	if (ret) {
		devres_release_group(&hdev->dev, hdev->devres_group_id);
		hid_close_report(hdev);
		hdev->driver = NULL;
	}

The hidraw character device that hid_hw_start() registered through
hid_connect() is allocated with kzalloc() and added with cdev_device_add(),
so it is not devres-managed and survives that.  With hdev->driver NULL,
hid_device_remove() skips hid_hw_stop() as well, because it only unwinds
while a driver is still attached.  The registration therefore outlives the
device on both paths.

Opening the surviving /dev/hidrawX writes into freed memory.  KASAN reports
a use-after-free write from hidraw_open() -> hid_hw_open() -> the
transport's open callback, which takes a spinlock inside the freed object.
A descriptor that carries a PID usage page and no input reports is enough:
hidraw claims the device so hid_hw_start() succeeds, while hid->inputs
stays empty so force-feedback init fails.  The other failure returns in
hid_pidff_init_with_quirks() - no output reports, an allocation failure,
pidff_init_fields(), pidff_check_autocenter(), an unusable effect count,
input_ff_create() - all reach the same label.

Stop the device on that path.  hid-dr.c and hid-emsff.c, which start the
device with the same HID_CONNECT_DEFAULT & ~HID_CONNECT_FF mask, already do
this.  The two earlier gotos must keep returning without hid_hw_stop(),
since neither has a started device, so give the path that fails after the
start its own label.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: f06bf8d94fff ("HID: Add hid-universal-pidff driver and supported device ids")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 drivers/hid/hid-universal-pidff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-universal-pidff.c b/drivers/hid/hid-universal-pidff.c
index 549dac555d40..60180467a0bf 100644
--- a/drivers/hid/hid-universal-pidff.c
+++ b/drivers/hid/hid-universal-pidff.c
@@ -104,12 +104,14 @@ static int universal_pidff_probe(struct hid_device *hdev,
 	error = init_function(hdev, id->driver_data);
 	if (error) {
 		hid_warn(hdev, "Error initialising force feedback\n");
-		goto err;
+		goto err_stop;
 	}
 
 	hid_info(hdev, "Universal pidff driver loaded successfully!");
 
 	return 0;
+err_stop:
+	hid_hw_stop(hdev);
 err:
 	return error;
 }
-- 
2.53.0

Re: [PATCH] HID: universal-pidff: stop the device when force-feedback init fails

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing validation of input reports in `universal_pidff_probe` causes an out-of-bounds heap write.
--

--- Patch [1]: [PATCH] HID: universal-pidff: stop the device when force-feedback init fails ---
commit 551db56065200f5b5fc8ed4d3eebe2e09f3793b4
Author: Baul Lee <baul.lee@xbow.com>

HID: universal-pidff: stop the device when force-feedback init fails

This commit fixes a use-after-free issue when force-feedback initialization
fails. It ensures the hardware is properly stopped if initialization fails,
preventing the hidraw character device from outliving the device and writing
into freed memory on subsequent opens.

> diff --git a/drivers/hid/hid-universal-pidff.c b/drivers/hid/hid-universal-pidff.c
> --- a/drivers/hid/hid-universal-pidff.c
> +++ b/drivers/hid/hid-universal-pidff.c
> @@ -104,12 +104,14 @@ static int universal_pidff_probe(struct hid_device *hdev,
>  	error = init_function(hdev, id->driver_data);

[Severity: Critical]
This isn't a bug introduced by this patch, but does calling init_function()
here without verifying if the inputs list is empty risk an out-of-bounds
write?

If a crafted USB device exposes a PID usage page and output reports but no
input reports, hdev->inputs remains empty. When hid_pidff_init_with_quirks()
is called without inputs:

drivers/hid/usbhid/hid-pidff.c:hid_pidff_init_with_quirks() {
    struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
    struct input_dev *dev = hidinput->input;
    ...
}

Because the inputs list is empty, list_entry() points to the list head
itself. Due to the struct layouts of hid_device and hid_input, fetching
hidinput->input aliases directly to hid->hidraw.

This pointer to a much smaller struct hidraw object is then passed as a
struct input_dev pointer to pidff_init_fields().

If the descriptor contains the required output reports to pass
pidff_reports_ok(), can operations like this write out of bounds?

drivers/hid/usbhid/hid-pidff.c:pidff_init_fields() {
    if (pidff->type_id[PID_CONSTANT])
        set_bit(FF_CONSTANT, dev->ffbit);
    ...
}

>  	if (error) {
>  		hid_warn(hdev, "Error initialising force feedback\n");
> -		goto err;
> +		goto err_stop;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729142432.16184-1-baul.lee@xbow.com?part=1