Re: [PATCH v2] HID: hid-oxp: fix UAF on pending work in remove()

On Tue, Aug 04, 2026 at 02:24:14PM -0700, Derek John Clark wrote:
> On Tue, Aug 4, 2026 at 1:06 PM Shengzhuo Wei <[email protected]> wrote:
> >
> > On 2026-08-04 17:50, Shengzhuo Wei wrote:
> > > ---
> > >  drivers/hid/hid-oxp.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> > > index 20a54f337220dc2aee3483a14d542b66c487bd60..abd622ff1b26b312ad9c8a4375822832f8371533 100644
> > > --- a/drivers/hid/hid-oxp.c
> > > +++ b/drivers/hid/hid-oxp.c
> > > @@ -1501,14 +1501,14 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
> > >       drvdata.gamepad_mode = OXP_GP_MODE_XINPUT;
> > >       drvdata.rumble_intensity = 5;
> > >
> > > -     INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
> > > -     mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
> > > -
> > >       ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
> > >       if (ret)
> > >               return dev_err_probe(&hdev->dev, ret,
> > >                                    "Failed to attach configuration attributes\n");
> > >
> > > +     INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
> > > +     mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1552,9 +1552,9 @@ static int oxp_hid_probe(struct hid_device *hdev,
> > >
> > >  static void oxp_hid_remove(struct hid_device *hdev)
> > >  {
> > > -     cancel_delayed_work(&drvdata.oxp_rgb_queue);
> > > -     cancel_delayed_work(&drvdata.oxp_btn_queue);
> > > -     cancel_delayed_work(&drvdata.oxp_mcu_init);
> > > +     disable_delayed_work_sync(&drvdata.oxp_rgb_queue);
> > > +     disable_delayed_work_sync(&drvdata.oxp_btn_queue);
> > > +     disable_delayed_work_sync(&drvdata.oxp_mcu_init);
> > >       hid_hw_close(hdev);
> > >       hid_hw_stop(hdev);
> > >  }
> > >
> > > ---
> >
> > Hi Dmitry,
> >
> > Thanks again for the v1 review. Sashiko's v2 review raised two points I
> > want to act on:
> >
> > 1. "Uninitialized work struct access during early raw events" -- agreed.
> >    v2 moved INIT_DELAYED_WORK(&drvdata.oxp_mcu_init) to the end of
> >    oxp_cfg_probe(), widening the window in which an early status report
> >    could mod_delayed_work() a not-yet-initialized (zeroed) work. I'll
> >    fix this in v3 by keeping INIT_DELAYED_WORK() before
> >    devm_device_add_group() and moving only the mod_delayed_work() after
> >    it: the work is then initialized before any raw event can arm it,
> >    while a probe failure still can't leave it armed.
> >
> Hi Shengzhuo,
> 
> I dealt with this when writing the hid-msi drivers as well, and
> Sashiko will continuously come up with new issues for each revision.

Yes, so we do not require patch submitters to act on all Sashiko
findings (and quite often they are not right), especially pre-existing
ones. So feel free to ignore this.

For this particular instance maybe simply drop this change from v3: it
is all broken anyways. The driver should initialize all internal
structures (work items, lock, etc) beforehand and then start configuring
the behavior since events can be coming in at any moment.

And on probe error it should try to clean up everything (again, on the

> The next issue is going to be that the attributes are initialized
> before the setup function has run to query the device after the MCU
> will accept messages. The solution I found was to move
> devm_device_add_group to the end of the init work queue once the query
> has been completed, then inform sysfs with a `change` uevent. Perhaps
> that pattern will be useful here.
> 
> For ease of reviewing, below is the cfg_setup_fn from that driver
> 
> > 2. "Global workqueues permanently disabled on inert interface removal."
> >    The driver keeps all state in a single static global drvdata, and
> >    module_hid_driver() binds it to every interface of the device, so
> >    oxp_hid_remove() runs when any interface is unbound. With
> >    disable_delayed_work_sync() and no enable_delayed_work() anywhere,
> >    unbinding an inert interface disables the works for the still-bound
> >    gamepad interface. cancel_delayed_work_sync() (v1) re-enabled them,
> >    so it didn't have this side effect.
> >
> >    This looks like a symptom of the static-global-drvdata issue rather
> >    than disable_delayed_work_sync() itself -- with per-device drvdata
> >    each interface would have its own works. Before I send v3, would you
> >    prefer to keep disable_delayed_work_sync() (and address the
> >    multi-interface case via the per-device drvdata refactor you
> >    mentioned as a separate patch), or go back to
> >    cancel_delayed_work_sync()? I'll hold v3 until I hear from you.
> >

Yes, so feel free to ignore Sashiko or (if you have spare cycles)
address this global instance in a separate patch at some later time.

> 
> I had planned on devm_alloc the drvdata per device once the msi
> drivers are done. If you are willing to take that on I'll be glad to
> test with my F1 Pro. Out of curiosity, do you have a device available
> to test as well? If so, which model do you have? I can ask the
> community to test on any device family that isn't covered.
> 
> To your question, I don't think the bug fix switching to
> disable_delayed_work_sync is worth the side effects without first
> addressing the global drvdata issue. That would introduce a true
> regression to fix a theoretical logic bug. I would rather you either
> switch to the devm_alloc drvdata first or hold that change until I'm
> able to do it later myself.

If there is possibility to have multiple instances then this driver is
FUBAR in the current shape: one device instance fires up work items for
another device. So holding the patch makes no sense IMO.

Thanks.

-- 
Dmitry