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

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 [off-list ref]

    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.

[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