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

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

[Shengzhuo Wei]

oxp_cfg_probe() arms drvdata.oxp_mcu_init to run 50 ms after probe, and
oxp_mcu_init_fn() dereferences drvdata.hdev to issue MCU output reports
(oxp_set_buttons()/oxp_gen_2_property_out() -> hid_hw_output_report(),
each followed by msleep(200)). oxp_hid_remove() cancels it with the
non-synchronising cancel_delayed_work(), so a worker already running is
not waited for; removing the device while the worker is asleep then frees
the hid_device underneath it, leaving drvdata.hdev stale -- a
use-after-free when the worker wakes.

The oxp_rgb_queue and oxp_btn_queue workers, wired up the same way and
also cancelled with cancel_delayed_work() in oxp_hid_remove(), have the
same problem.

Drain all three works with cancel_delayed_work_sync() in oxp_hid_remove()
so they have exited before the hid_device is freed.

Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
Fixes: e4c850a6e750 ("HID: hid-oxp: Add Button Mapping Interface")
Fixes: 2f424f28fb39 ("HID: hid-oxp: Add Second Generation Gamepad Mode Switch")
Cc: stable@vger.kernel.org
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
Same delayed-work use-after-free class as the 7.2-rc6 sweep
(hid-lenovo-go, hid-lenovo-go-s, hid-lg-g15, hid-appleir, hid-letsketch);
hid-oxp was missed. The fix mirrors the cancel_delayed_work_sync()
approach already used by hid-lenovo-go / hid-lenovo-go-s.
---
 drivers/hid/hid-oxp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 20a54f337220dc2aee3483a14d542b66c487bd60..d8fb6a69d40d43f2595179df1067d42b4b3e166a 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -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);
+	cancel_delayed_work_sync(&drvdata.oxp_rgb_queue);
+	cancel_delayed_work_sync(&drvdata.oxp_btn_queue);
+	cancel_delayed_work_sync(&drvdata.oxp_mcu_init);
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }

---
base-commit: 075b74841bd0065a3bda3440873c747938e69b68
change-id: 20260804-oxp-fix-879390c5e47f

Best regards,
-- 
Shengzhuo Wei <me@cherr.cc>

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The transition to `cancel_delayed_work_sync()` is ineffective because the delayed works can be immediately re-queued after cancellation, leading to the same use-after-free the patch attempts to fix.
- [High] The driver stores per-device state in a single static global `drvdata` structure instead of using per-device contexts, leading to memory corruption, timer list corruption, and Use-After-Free if multiple matching devices or interfaces are probed.
- [High] Delayed works are leaked and will execute on a freed `hid_device` if `oxp_cfg_probe()` encounters an error after arming the work.
--

--- Patch [1]: [PATCH] HID: hid-oxp: fix UAF on pending work in remove() ---
commit 9da1adfd8fd14b4d1b17ae38165e5f4aa4ff44f5
Author: Shengzhuo Wei <me@cherr.cc>

    HID: hid-oxp: fix UAF on pending work in remove()
    
    Drain oxp_rgb_queue, oxp_btn_queue, and oxp_mcu_init works with
    cancel_delayed_work_sync() in oxp_hid_remove() so they exit before the
    hid_device is freed.

> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 20a54f337220d..d8fb6a69d40d4 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -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);
> +	cancel_delayed_work_sync(&drvdata.oxp_rgb_queue);
> +	cancel_delayed_work_sync(&drvdata.oxp_btn_queue);
> +	cancel_delayed_work_sync(&drvdata.oxp_mcu_init);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the works
to be re-armed immediately after they are cancelled here?

Since hid_hw_stop() is called after the cancellations, hardware events can
still arrive and trigger oxp_hid_raw_event_gen_2(), which could re-arm
oxp_mcu_init.

Similarly, since devres cleans up sysfs attributes and the LED class device
after remove() returns, userspace could still trigger oxp_rgb_brightness_set()
and re-arm oxp_rgb_queue:

drivers/hid/hid-oxp.c:oxp_rgb_brightness_set() {
    ...
    mod_delayed_work(system_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
}

Could this lead to the same use-after-free this patch is attempting to fix
by allowing the works to run on a freed hid_device?

[Severity: High]
This is a pre-existing issue, but does using a single static global drvdata
structure cause memory corruption if multiple matching devices are probed?

When oxp_cfg_probe() runs, it unconditionally re-initializes the global state:

drivers/hid/hid-oxp.c:oxp_cfg_probe() {
    ...
    drvdata.hdev = hdev;
    ...
    INIT_DELAYED_WORK(&drvdata.oxp_rgb_queue, oxp_rgb_queue_fn);
}

If a second device (or a composite device with multiple matching interfaces)
is connected, calling INIT_DELAYED_WORK() on an already active work item
could corrupt the workqueue linked lists.

Additionally, removing one device would cancel the workers for the other
while leaving drvdata.hdev pointing to a freed device.

[Severity: High]
This is also a pre-existing issue, but does the probe error path leak the
delayed works?

In oxp_cfg_probe(), the work is armed before all resources are guaranteed to
be initialized:

drivers/hid/hid-oxp.c:oxp_cfg_probe() {
    ...
    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(...);
}

If devm_device_add_group() fails, oxp_cfg_probe() returns an error. The caller
oxp_hid_probe() handles this by calling hid_hw_close() and hid_hw_stop(), but
it does not appear to cancel the pending delayed works before the HID core
frees the device.

Will the delayed work execute on a freed hid_device when the timer expires in
this error scenario?

>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-oxp-fix-v1-1-51a4fe787167@cherr.cc?part=1

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

[Dmitry Torokhov]

Hi Shengzhuo,

On Tue, Aug 04, 2026 at 07:15:32AM +0800, Shengzhuo Wei wrote:
> oxp_cfg_probe() arms drvdata.oxp_mcu_init to run 50 ms after probe, and
> oxp_mcu_init_fn() dereferences drvdata.hdev to issue MCU output reports
> (oxp_set_buttons()/oxp_gen_2_property_out() -> hid_hw_output_report(),
> each followed by msleep(200)). oxp_hid_remove() cancels it with the
> non-synchronising cancel_delayed_work(), so a worker already running is
> not waited for; removing the device while the worker is asleep then frees
> the hid_device underneath it, leaving drvdata.hdev stale -- a
> use-after-free when the worker wakes.
> 
> The oxp_rgb_queue and oxp_btn_queue workers, wired up the same way and
> also cancelled with cancel_delayed_work() in oxp_hid_remove(), have the
> same problem.
> 
> Drain all three works with cancel_delayed_work_sync() in oxp_hid_remove()
> so they have exited before the hid_device is freed.
> 
> Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
> Fixes: e4c850a6e750 ("HID: hid-oxp: Add Button Mapping Interface")
> Fixes: 2f424f28fb39 ("HID: hid-oxp: Add Second Generation Gamepad Mode Switch")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shengzhuo Wei <me@cherr.cc>
> ---
> Same delayed-work use-after-free class as the 7.2-rc6 sweep
> (hid-lenovo-go, hid-lenovo-go-s, hid-lg-g15, hid-appleir, hid-letsketch);
> hid-oxp was missed. The fix mirrors the cancel_delayed_work_sync()
> approach already used by hid-lenovo-go / hid-lenovo-go-s.
> ---
>  drivers/hid/hid-oxp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 20a54f337220dc2aee3483a14d542b66c487bd60..d8fb6a69d40d43f2595179df1067d42b4b3e166a 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -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);
> +	cancel_delayed_work_sync(&drvdata.oxp_rgb_queue);
> +	cancel_delayed_work_sync(&drvdata.oxp_btn_queue);
> +	cancel_delayed_work_sync(&drvdata.oxp_mcu_init);

What stops these jobs from re-arming? Should it use
disable_delayed_work_sync() instead?

Thanks.

-- 
Dmitry

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

[Shengzhuo Wei]

On 2026-08-03 16:33, Dmitry Torokhov wrote:
 
> What stops these jobs from re-arming? Should it use
> disable_delayed_work_sync() instead?

Agreed — cancel_delayed_work_sync() only drains the running instance; it
doesn't stop the works from being re-queued from the raw_event / sysfs /
LED-brightness paths before the device is fully torn down, so the UAF
window isn't closed. I'll switch all three calls to
disable_delayed_work_sync() in v2.

Do you have any other comments you'd like addressed in v2?

Thanks,
Shengzhuo

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

[Dmitry Torokhov]

On Tue, Aug 04, 2026 at 07:54:57AM +0800, Shengzhuo Wei wrote:
> On 2026-08-03 16:33, Dmitry Torokhov wrote:
>  
> > What stops these jobs from re-arming? Should it use
> > disable_delayed_work_sync() instead?
> 
> Agreed — cancel_delayed_work_sync() only drains the running instance; it
> doesn't stop the works from being re-queued from the raw_event / sysfs /
> LED-brightness paths before the device is fully torn down, so the UAF
> window isn't closed. I'll switch all three calls to
> disable_delayed_work_sync() in v2.
> 
> Do you have any other comments you'd like addressed in v2?

Since you are addressing work-related UAF, fixing it on probe failures
would be nice (Sashiko mentioned it).

The other Sahiko comment regarding global instance would be nice to get
fixed as well (but separate patch and only if you have
time/inclination).

Another potential change is not to create device attribute group by hand
but attach it to the driver structure so it is created and torn down
automatically, and control visibility of the attributes through
is_visible() method.

Thanks.

-- 
Dmitry