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

On Tue, Aug 4, 2026 at 1:06=E2=80=AFPM 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..abd622ff1b26b312ad9c8a4=
375822832f8371533 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 *hde=
v, u16 up)
> >       drvdata.gamepad_mode =3D OXP_GP_MODE_XINPUT;
> >       drvdata.rumble_intensity =3D 5;
> >
> > -     INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
> > -     mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffi=
es(50));
> > -
> >       ret =3D devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
> >       if (ret)
> >               return dev_err_probe(&hdev->dev, ret,
> >                                    "Failed to attach configuration attr=
ibutes\n");
> >
> > +     INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
> > +     mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffi=
es(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.
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.
>

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.

Thanks,
Derek

---

static void cfg_setup_fn(struct work_struct *work)
{
    struct delayed_work *dwork =3D container_of(work, struct delayed_work, =
work);
    struct claw_drvdata *drvdata =3D container_of(dwork, struct
claw_drvdata, cfg_setup);
    bool gamepad_ready =3D false, rgb_ready =3D false, gp_registered,
rgb_registered;
    int ret;

    ret =3D claw_hw_output_report(drvdata->hdev,
CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE,
                    NULL, 0, 25);
    if (ret) {
        dev_err(&drvdata->hdev->dev,
            "Failed to read gamepad mode: %d\n", ret);
        goto prep_rgb;
    }
    gamepad_ready =3D true;

prep_rgb:
    ret =3D claw_read_rgb_config(drvdata->hdev);
    if (ret) {
        dev_err(&drvdata->hdev->dev,
            "Failed to read RGB config: %d\n", ret);
        goto try_gamepad;
    }
    rgb_ready =3D true;

    /* Add sysfs attributes after we get the device state */
try_gamepad:
    scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
        /* Pairs with smp_store_release from below */
        gp_registered =3D smp_load_acquire(&drvdata->gp_registered);

    if (!gp_registered && gamepad_ready) {
        ret =3D device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_gr=
oup);
        if (ret) {
            dev_err(&drvdata->hdev->dev,
                "Failed to create gamepad attrs: %d\n", ret);
            goto try_rgb;
        }

        scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
            /* Pairs with smp_load_acquire in attribute show/store function=
s */
            smp_store_release(&drvdata->gp_registered, true);
            gp_registered =3D true;
        }
    }

try_rgb:
    /* Add and enable RGB interface once we have the device state */
    scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
        /* Pairs with smp_store_release from below */
        rgb_registered =3D smp_load_acquire(&drvdata->rgb_registered);

    if (!rgb_registered && rgb_ready) {
        ret =3D led_classdev_multicolor_register(&drvdata->hdev->dev,
                               &drvdata->led_mc);
        if (ret) {
            dev_err(&drvdata->hdev->dev,
                "Failed to create led device: %d\n", ret);
            goto update_kobjects;
        }

        ret =3D device_add_group(drvdata->led_mc.led_cdev.dev,
&claw_rgb_attr_group);
        if (ret) {
            dev_err(&drvdata->hdev->dev,
                "Failed to create RGB attrs: %d\n", ret);
            led_classdev_multicolor_unregister(&drvdata->led_mc);
            goto update_kobjects;
        }

        scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
            /* Pairs with smp_load_acquire in attribute show/store function=
s */
            smp_store_release(&drvdata->rgb_registered, true);
            rgb_registered =3D true;
        }
    }

update_kobjects:
    if (gp_registered)
        kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
    if (rgb_registered)
        kobject_uevent(&drvdata->led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
}
> The remaining sashiko items look pre-existing and outside this fix.
>
> Thanks,
> Shengzhuo