[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

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

[Tomasz Pakuła]

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 <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;
>  }

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

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

[Baul Lee]

Thanks for looking at it.  I agree with the rule, but I don't think those two
exits can take the stop, and I would rather explain why than send a v2 that
changes them.

After hid_hw_start() there is exactly one error return in
universal_pidff_probe(), the init_function() one this patch changes.  The two
exits on lines 92 and 101 return 0, which means the probe succeeds, and that
is the whole difference between them.  Because they succeed, hdev->driver
stays set, and since universal_pidff has no .remove callback,
hid_device_remove() falls back to the default remove, which is hid_hw_stop().
The device those two exits leave running is therefore already stopped for us
when it is unbound or unplugged, and nothing leaks there.  The leak exists
only on the path where the probe fails, because __hid_device_probe() clears
hdev->driver on failure and hid_device_remove() then skips the unwind
entirely.

If we call hid_hw_stop() on those two exits as well, it ends up running twice
on the same device, and usbhid_stop() does not survive that.  It clears the
three URB pointers on the way out - that is what the "don't mess up next
start" comment is about - but the buffers it releases immediately afterwards
through hid_free_buffers() are left dangling: inbuf, outbuf and ctrlbuf go
back through usb_free_coherent() and usbhid->cr through kfree(), and none of
those four pointers is cleared.  There is no HID_STARTED check at the top of
usbhid_stop() either, so the second call frees all four again.  Every unplug
of a device that took one of those exits would be a double free, which is a
good deal worse than the leak this patch is fixing.

The other problem is that stopping there takes down the very device those
exits exist for.  hid_hw_stop() calls hid_disconnect(), which unregisters the
input device through hidinput_disconnect() and the character device through
hidraw_disconnect().  The additional "device" you mention, the one that is
there just for the extra buttons and axes, is by definition the one without a
PID usage page in its descriptor, so it is exactly the one that takes the exit
on line 92.  Stopping it would leave this driver bound to a device with no
input node and no hidraw node at all, and returning an error there instead of
0 does not help either, because hid-generic will not adopt the interface:
hid_generic_match() walks the bus and gives up as soon as it finds another
driver that matches the device.  Either way the buttons and axes disappear,
which I do not think is what you are after.

2026년 8월 3일 (월) 오전 5:41, Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>님이 작성:
>
> 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 <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;
> >  }
>
> 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
>

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

[Tomasz Pakuła]

On Tue, 2026-08-04 at 14:38 +0900, Baul Lee wrote:
> Thanks for looking at it.  I agree with the rule, but I don't think those two
> exits can take the stop, and I would rather explain why than send a v2 that
> changes them.
> 
> After hid_hw_start() there is exactly one error return in
> universal_pidff_probe(), the init_function() one this patch changes.  The two
> exits on lines 92 and 101 return 0, which means the probe succeeds, and that
> is the whole difference between them.  Because they succeed, hdev->driver
> stays set, and since universal_pidff has no .remove callback,
> hid_device_remove() falls back to the default remove, which is hid_hw_stop().
> The device those two exits leave running is therefore already stopped for us
> when it is unbound or unplugged, and nothing leaks there.  The leak exists
> only on the path where the probe fails, because __hid_device_probe() clears
> hdev->driver on failure and hid_device_remove() then skips the unwind
> entirely.
> 
> If we call hid_hw_stop() on those two exits as well, it ends up running twice
> on the same device, and usbhid_stop() does not survive that.  It clears the
> three URB pointers on the way out - that is what the "don't mess up next
> start" comment is about - but the buffers it releases immediately afterwards
> through hid_free_buffers() are left dangling: inbuf, outbuf and ctrlbuf go
> back through usb_free_coherent() and usbhid->cr through kfree(), and none of
> those four pointers is cleared.  There is no HID_STARTED check at the top of
> usbhid_stop() either, so the second call frees all four again.  Every unplug
> of a device that took one of those exits would be a double free, which is a
> good deal worse than the leak this patch is fixing.
> 
> The other problem is that stopping there takes down the very device those
> exits exist for.  hid_hw_stop() calls hid_disconnect(), which unregisters the
> input device through hidinput_disconnect() and the character device through
> hidraw_disconnect().  The additional "device" you mention, the one that is
> there just for the extra buttons and axes, is by definition the one without a
> PID usage page in its descriptor, so it is exactly the one that takes the exit
> on line 92.  Stopping it would leave this driver bound to a device with no
> input node and no hidraw node at all, and returning an error there instead of
> 0 does not help either, because hid-generic will not adopt the interface:
> hid_generic_match() walks the bus and gives up as soon as it finds another
> driver that matches the device.  Either way the buttons and axes disappear,
> which I do not think is what you are after.
> 
> 2026년 8월 3일 (월) 오전 5:41, Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>님이 작성:
> > 
> > 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 <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;
> > >  }
> > 
> > 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
> > 

Okay! Thanks for the detailed explanation and for the contribution. I
tested this by mangling PID descriptor and indeed, like you said, it
would completely disable input from the steering wheelso the current
version is proper.

-- 
Reviewed-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>