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

On Wed, 2026-07-29 at 23:24 +0900, Baul Lee wrote:
> 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 <[email protected]>
> 
> Fixes: f06bf8d94fff ("HID: Add hid-universal-pidff driver and supported device ids")
> Cc: [email protected]
> Signed-off-by: Baul Lee <[email protected]>
> ---
>  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;
>  }

Actually, now that I look at it, this isn't the only place where the
stop should happen. Any error after successful hid_hw_start() should
trigger hid_hw_stop().

So for this to truly matter, you should replace
	return 0;
with
	goto err_stop;
on lines 92 and 101. Just add error value clearing so error = 0 because
even though we're exiting there, it's not an error. Some devices just
show up as multiple separate HID devices.

Tomasz