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

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 th=
at
> changes them.
>=20
> 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 t=
hat
> 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_sto=
p().
> 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 exist=
s
> only on the path where the probe fails, because __hid_device_probe() clea=
rs
> hdev->driver on failure and hid_device_remove() then skips the unwind
> entirely.
>=20
> If we call hid_hw_stop() on those two exits as well, it ends up running t=
wice
> on the same device, and usbhid_stop() does not survive that.  It clears t=
he
> 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 afterwa=
rds
> through hid_free_buffers() are left dangling: inbuf, outbuf and ctrlbuf g=
o
> 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 unp=
lug
> 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.
>=20
> 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 throu=
gh
> hidraw_disconnect().  The additional "device" you mention, the one that i=
s
> there just for the extra buttons and axes, is by definition the one witho=
ut 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 n=
o
> input node and no hidraw node at all, and returning an error there instea=
d 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 anothe=
r
> driver that matches the device.  Either way the buttons and axes disappea=
r,
> which I do not think is what you are after.
>=20
> 2026=EB=85=84 8=EC=9B=94 3=EC=9D=BC (=EC=9B=94) =EC=98=A4=EC=A0=84 5:41, =
Tomasz Paku=C5=82a <[email protected]>=EB=8B=98=EC=9D=B4 =
=EC=9E=91=EC=84=B1:
> >=20
> > On Wed, 2026-07-29 at 23:24 +0900, Baul Lee wrote:
> > > universal_pidff_probe() starts the device with hid_hw_start() and the=
n, if
> > > force-feedback initialisation fails, returns the error through a labe=
l that
> > > only does "return error".  The device is left started.
> > >=20
> > > The HID core does not unwind on the driver's behalf.  __hid_device_pr=
obe()
> > > releases the devres group, closes the report and clears hdev->driver:
> > >=20
> > >       if (ret) {
> > >               devres_release_group(&hdev->dev, hdev->devres_group_id)=
;
> > >               hid_close_report(hdev);
> > >               hdev->driver =3D NULL;
> > >       }
> > >=20
> > > 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 NUL=
L,
> > > hid_device_remove() skips hid_hw_stop() as well, because it only unwi=
nds
> > > while a driver is still attached.  The registration therefore outlive=
s the
> > > device on both paths.
> > >=20
> > > Opening the surviving /dev/hidrawX writes into freed memory.  KASAN r=
eports
> > > a use-after-free write from hidraw_open() -> hid_hw_open() -> the
> > > transport's open callback, which takes a spinlock inside the freed ob=
ject.
> > > A descriptor that carries a PID usage page and no input reports is en=
ough:
> > > hidraw claims the device so hid_hw_start() succeeds, while hid->input=
s
> > > stays empty so force-feedback init fails.  The other failure returns =
in
> > > hid_pidff_init_with_quirks() - no output reports, an allocation failu=
re,
> > > pidff_init_fields(), pidff_check_autocenter(), an unusable effect cou=
nt,
> > > input_ff_create() - all reach the same label.
> > >=20
> > > 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, alre=
ady 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.
> > >=20
> > > Discovered by XBOW, triaged by Baul Lee <[email protected]>
> > >=20
> > > Fixes: f06bf8d94fff ("HID: Add hid-universal-pidff driver and support=
ed 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(-)
> > >=20
> > > diff --git a/drivers/hid/hid-universal-pidff.c b/drivers/hid/hid-univ=
ersal-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_dev=
ice *hdev,
> > >       error =3D init_function(hdev, id->driver_data);
> > >       if (error) {
> > >               hid_warn(hdev, "Error initialising force feedback\n");
> > > -             goto err;
> > > +             goto err_stop;
> > >       }
> > >=20
> > >       hid_info(hdev, "Universal pidff driver loaded successfully!");
> > >=20
> > >       return 0;
> > > +err_stop:
> > > +     hid_hw_stop(hdev);
> > >  err:
> > >       return error;
> > >  }
> >=20
> > 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().
> >=20
> > 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 =3D 0 becau=
se
> > even though we're exiting there, it's not an error. Some devices just
> > show up as multiple separate HID devices.
> >=20
> > Tomasz
> >=20

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.

--=20
Reviewed-by: Tomasz Paku=C5=82a <[email protected]>