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

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.

[ ... ]

[ ... ]

 	/* 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.

[ ... ]

[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/20260817213840.1053216-1-rafael@rcpassos.me?part=4