Re: [PATCH v4 4/4] HID: wiimote: wiimote_probe with scoped cleanup

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

New issues:
- [Critical] Skipping cancellation of `init_worker` and `timer` on probe error paths due to a conditional check leads to a Use-After-Free.
- [High] Non-atomic bitwise updates to `wdata->init_state` across concurrent threads cause a data race, leading to lost state flags and resource leaks.

Pre-existing issues:
- [Critical] Incorrect teardown ordering in `wiimote_destroy()` cancels `queue.worker` before stopping hardware, allowing the worker to be re-queued after cancellation.
--

--- Patch [4]: [PATCH v4 4/4] HID: wiimote: wiimote_probe with scoped cleanup ---
commit b9fae511d83189a3adfc0824b46530dbaab4dda2
Author: Rafael Passos <[email protected]>

HID: wiimote: wiimote_probe with scoped cleanup

This commit introduces scoped cleanup to the wiimote driver, adding a
new bitmask to track the probing state. It updates the probe and destroy
functions to conditionally clean up resources based on this state.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c

[ ... ]

> @@ -1751,39 +1757,56 @@ static DEFINE_IDA(wiimote_ida);
>  
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
[ ... ]
>  	/* prevent init_worker from being scheduled again */
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock)
>  		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
>  
> -	cancel_work_sync(&wdata->init_worker);
> -	timer_shutdown_sync(&wdata->timer);
> +	if (wdata->init_state & WIIMOTE_PROBE_READY) {
> +		cancel_work_sync(&wdata->init_worker);
> +		timer_shutdown_sync(&wdata->timer);
> +	}

[Severity: Critical]
If probe fails after creating the sysfs nodes but before setting
WIIMOTE_PROBE_READY, can this lead to a use-after-free?

In wiimote_hid_probe(), the sysfs node for extension is created before
initialization finishes:

wiimote_hid_probe() {
        ...
        ret = device_create_file(&hdev->dev, &dev_attr_extension);
        ...
}

If userspace writes to this node, it can trigger wiimote_schedule(), placing
init_worker on the workqueue. If a subsequent step in probe fails,
wiimote_destroy() is called. Since WIIMOTE_PROBE_READY is not yet set, the
worker and timer cancellations are skipped, and wdata is freed while the
worker is still queued.

[ ... ]

> +	if (wdata->init_state & WIIMOTE_MODULES_LOADED)
> +		wiimote_modules_unload(wdata);
> +
>  	cancel_work_sync(&wdata->queue.worker);
> -	hid_hw_close(wdata->hdev);
> -	hid_hw_stop(wdata->hdev);
> +
> +	if (wdata->init_state & WIIMOTE_PROBE_HW_OPENED)
> +		hid_hw_close(wdata->hdev);
> +	if (wdata->init_state & WIIMOTE_PROBE_HW_STARTED)
> +		hid_hw_stop(wdata->hdev);
>  
>  	kfree(wdata);
>  }

[Severity: Critical]
This is a pre-existing issue, but does this teardown order allow
queue.worker to be re-queued after it is cancelled?

Here, cancel_work_sync(&wdata->queue.worker) is called before hid_hw_stop().
Because the hardware is still active, incoming HID reports can trigger
wiimote_hid_event(). Event handlers can queue output commands via
wiimote_queue(), which calls schedule_work(&wdata->queue.worker).

This could place the worker back on the system workqueue just before wdata
is freed, leading to a use-after-free when the worker executes.

[ ... ]

> @@ -1834,24 +1859,10 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  
>  	/* schedule device detection */
>  	wiimote_schedule(wdata);
> +	wdata->init_state |= WIIMOTE_PROBE_READY;
>  
> +	retain_and_null_ptr(wdata);
>  	return 0;

[Severity: High]
Can these non-atomic bitwise updates to wdata->init_state cause data races?

Here, wiimote_schedule(wdata) queues init_worker. Immediately after, this
thread updates wdata->init_state |= WIIMOTE_PROBE_READY.

Concurrently, init_worker executes on another CPU and calls functions like
wiimote_modules_load(), which perform their own non-atomic updates:

wiimote_modules_load() {
        ...
        wdata->init_state |= WIIMOTE_MODULES_LOADED;
}

Since init_state is a u8, these non-atomic read-modify-write operations can
race. If the worker's update overwrites the probe thread's update,
WIIMOTE_PROBE_READY is lost, skipping worker cancellation during teardown.
If the probe thread overwrites the worker's update, WIIMOTE_MODULES_LOADED
is lost, potentially leaking devices on unbind.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4