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

[PATCH v3] 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
(hid_hw_output_report() followed by msleep(200)). The oxp_rgb_queue and
oxp_btn_queue workers are wired up the same way. oxp_hid_remove()
cancels all three with the non-synchronising cancel_delayed_work(), so a
worker already running is not waited for; removing the device while a
worker is asleep then frees the hid_device underneath it, leaving
drvdata.hdev stale -- a use-after-free when the worker wakes.

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.
---
Changes in v3:
- Revert to cancel_delayed_work_sync() and drop v2's probe-change
  reordering: with the driver's static global drvdata,
  disable_delayed_work_sync() would permanently disable the works when
  any interface of the device is unbound (Derek J. Clark). The re-arm
  hardening and the per-device drvdata rework will be handled separately
  by the driver maintainer.
- Link to v2: https://lore.kernel.org/r/20260804-oxp-fix-v2-1-b2d56e4c8a2c@cherr.cc

Changes in v2:
- Use disable_delayed_work_sync() instead of cancel_delayed_work_sync()
  so the works cannot be re-armed (e.g. via oxp_rgb_brightness_set())
  while the device is being torn down (Dmitry Torokhov).
- Arm oxp_mcu_init only after devm_device_add_group() succeeds, so a
  probe failure can no longer leave it pending to fire on a freed
  hid device (sashiko).
- Link to v1: https://lore.kernel.org/r/20260804-oxp-fix-v1-1-51a4fe787167@cherr.cc
---
 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 v3] HID: hid-oxp: fix UAF on pending work in remove()

[sashiko-bot]

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

New issues:
- [High] Calling `cancel_delayed_work_sync()` on uninitialized work structures triggers a kernel `WARN_ON`.

Pre-existing issues:
- [Critical] Global `drvdata` leads to cross-device memory corruption and broken isolation.
- [High] Missing delayed work cancellation in probe error path leads to Use-After-Free.
- [High] Re-arming of delayed works during and after `remove()` causes Use-After-Free.
- [High] Missing report size validation in raw event handlers leads to out-of-bounds memory read.
--

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

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

This commit addresses a use-after-free regression by changing
cancel_delayed_work() to cancel_delayed_work_sync() in oxp_hid_remove().
This ensures workers exit before the device is freed, preventing asleep
workers from waking up and dereferencing a stale hid_device.

Link: https://lore.kernel.org/r/20260804-oxp-fix-v2-1-b2d56e4c8a2c@cherr.cc
Link: https://lore.kernel.org/r/20260804-oxp-fix-v1-1-51a4fe787167@cherr.cc

> 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,

[Severity: Critical]
This isn't a bug introduced by this patch, but does using a single static global
structure for per-device state cause cross-device memory corruption?

When oxp_cfg_probe() runs for a new device, it unconditionally overwrites
drvdata.hdev, drvdata.bmap_1, and drvdata.bmap_2 with its own allocations:

oxp_cfg_probe()
    ...
    hid_set_drvdata(hdev, &drvdata);
    mutex_init(&drvdata.cfg_mutex);
    drvdata.hdev = hdev;
    ...
    drvdata.bmap_1 = bmap_1;
    drvdata.bmap_2 = bmap_2;

If multiple supported interfaces or gamepads are connected, won't they
clobber each other's state, leading to a use-after-free regression if one
is unbound while the other remains active?

[Severity: High]
This isn't a bug introduced by this patch, but could the probe error path leak
a running delayed work?

In oxp_cfg_probe(), drvdata.oxp_mcu_init is armed via mod_delayed_work().
If subsequent initialization like devm_device_add_group() fails, it
returns an error.

The error handler in oxp_hid_probe() cleans up hardware state but doesn't
cancel the pending delayed work before returning.

Could this allow the work to run after devres frees the device memory,
causing a use-after-free regression when oxp_mcu_init_fn() dereferences
the freed pointers?

[Severity: High]
This isn't a bug introduced by this patch, but do the raw event handlers
validate the incoming report size?

In both oxp_hid_raw_event_gen_1() and oxp_hid_raw_event_gen_2(), the code
casts the buffer and reads fields like rgb_rep->effect or checks indices
like data[3] without checking the size of the incoming HID report buffer.

If a device sends a truncated report, could this result in an out-of-bounds
memory read regression?

>  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]
If a device connects and exposes a standard HID interface, oxp_hid_probe()
can return success without calling oxp_cfg_probe(), leaving the work
structures uninitialized.

When the device is disconnected, oxp_hid_remove() will unconditionally
call cancel_delayed_work_sync() on these zero-initialized structures.

Could this trigger a WARN_ON(!work->func) inside __flush_work()?

[Severity: High]
This is a pre-existing issue, but modifying these lines highlights a
regression where cancellation happens before hid_hw_stop().

Because the hardware is not stopped, incoming USB reports can still invoke
oxp_hid_raw_event_gen_2(), which unconditionally calls mod_delayed_work().

Furthermore, sysfs attributes managed by devres remain accessible to
userspace until devres cleanup occurs after remove().

Can an unprivileged user writing to sysfs (like map_button_store) or
incoming hardware events re-arm the works after this synchronization,
causing a use-after-free regression when devres frees the memory?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-oxp-fix-v3-1-599b12c91a77@cherr.cc?part=1