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

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..abd622ff1b26b312ad9c8a4375822832f8371533 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 *hdev, u16 up)
>  	drvdata.gamepad_mode = OXP_GP_MODE_XINPUT;
>  	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));
> -
>  	ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
>  	if (ret)
>  		return dev_err_probe(&hdev->dev, ret,
>  				     "Failed to attach configuration attributes\n");
>  
> +	INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
> +	mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(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.

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.

The remaining sashiko items look pre-existing and outside this fix.

Thanks,
Shengzhuo