Re: [PATCH RFC 1/2] HID: hid-oxp: Replace system_wq with system_dfl_wq

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver uses a single global statically allocated LED class device, causing list corruption and state clobbering if multiple matching devices are connected.
- [High] Inadequate teardown of delayed works causes Use-After-Free vulnerabilities during device removal and probe failures.
- [High] Handling a GEN2 status event unconditionally dereferences `drvdata.led_mc`, which is NULL on hybrid MCU devices.
- [High] The driver blindly accesses HID event data without validating the `size` parameter, leading to out-of-bounds reads.
--

--- Patch [1]: [PATCH RFC 1/2] HID: hid-oxp: Replace system_wq with system_dfl_wq ---
commit 17346571c8ccd09d42318cd808c4c65570f51a89
Author: Marco Crivellari <marco.crivellari@suse.com>

    HID: hid-oxp: Replace system_wq with system_dfl_wq
    
    The function end up calling __queue_delayed_work(), which set a global
    timer that could fire anywhere, enqueuing the work where the timer fired.
    
    Unbound works could benefit from scheduler task placement, to optimize
    performance and power consumption.
    
    Since the workqueue work doesn't rely on per-cpu variables, there is no
    obvious reason that justify the use of a per-cpu workqueue. So change the
    workqueue with the new unbound version, system_dfl_wq.

> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 20a54f337220d..d2ded6b08ce9e 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -398,7 +398,7 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
>  	 * Re-apply our settings after this has been received.
>  	 */
>  	if (data[3] == OXP_EFFECT_MONO_TRUE) {
> -		mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
> +		mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));

[Severity: High]
This isn't a bug introduced by this patch, but does the driver validate the
size parameter before accessing data[0] or data[3] in this function?

It appears that if a device sends a smaller report than expected, we could hit
an out-of-bounds read when checking data[3] or when dumping OXP_PACKET_SIZE
bytes in oxp_hid_raw_event().

[Severity: High]
This is a pre-existing issue, but is it possible for drvdata.led_mc to be NULL
here?

If oxp_cfg_probe() jumps to skip_rgb for hybrid MCU devices, drvdata.led_mc
is left uninitialized (NULL). If the device later sends a GEN2 status event,
this function eventually does:

    led_mc->led_cdev.brightness = rgb_rep->brightness / 4 * ...

Could this cause a NULL pointer dereference on hybrid devices?

>  		return 0;
>  	}
>  
[ ... ]
> @@ -1349,7 +1349,7 @@ static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
>  				   enum led_brightness brightness)
>  {
>  	led_cdev->brightness = brightness;
> -	mod_delayed_work(system_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
> +	mod_delayed_work(system_dfl_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));

[Severity: High]
This is a pre-existing issue, but can this delayed work cause a use-after-free
during device removal?

Because oxp_hid_remove() uses cancel_delayed_work() instead of the synchronous
cancel_delayed_work_sync(), the work might still be running. Also, when devres
unregisters the LED classdev after remove() finishes, the LED core calls this
function, which unconditionally re-arms the workqueue.

Could the workqueue execute after devres has freed memory structures, leading
to a use-after-free?

>  }
>  
[ ... ]
> @@ -1502,7 +1502,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
>  	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));
> +	mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
>  
>  	ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
>  	if (ret)

[Severity: High]
This isn't a bug introduced by this patch, but if devm_device_add_group()
fails here, the probe function returns the error directly.

Does this leave the oxp_mcu_init delayed work scheduled, potentially causing a
use-after-free if the work runs after the device and its devres allocations
have been freed?

[Severity: Critical]
This is a pre-existing issue, but does oxp_cfg_probe() register a single
globally statically allocated led class device?

Earlier in oxp_cfg_probe():

    ret = devm_led_classdev_multicolor_register(&hdev->dev, &oxp_cdev_rgb);

Since oxp_cdev_rgb is statically allocated, if multiple matching HID devices
are connected, we will call devm_led_classdev_multicolor_register() multiple
times with the same pointer. Could this corrupt the LED subsystem's internal
linked list and cause a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707145327.313292-1-marco.crivellari@suse.com?part=1