Re: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Functions mix `goto`-based error handling cleanup labels with scope-based cleanup (`scoped_guard()`), violating the cleanup subsystem architectural rule.
- [Low] Incorrect 4-space indentation within `scoped_guard` blocks instead of standard kernel tabs.

Pre-existing issues:
- [High] Use-after-free of `wdata` because `queue.worker` can be rescheduled after it is cancelled in `wiimote_destroy()`.
--

--- Patch [2]: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit 343c163a7f9f1368f62e298a4bac0953c054d9f3
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: replace spinlock pairs with scoped_guard
    
Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 91c2cec35f6eb..d37d740a7162f 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -362,13 +363,12 @@ void wiiproto_req_rmem(struct wiimote_data *wdata, bool eeprom, __u32 offset,
>  int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset,
>  						const __u8 *wmem, __u8 size)
>  {
[ ... ]
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
> -	wiiproto_req_wreg(wdata, offset, wmem, size);
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +	    wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
> +	    wiiproto_req_wreg(wdata, offset, wmem, size);
> +	}

[Severity: Low]
this isn't a bug, but did the indentation get changed to 4 spaces here instead
of standard kernel tabs?

[ ... ]
> @@ -885,19 +874,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> -	wiiproto_req_status(wdata);
> -	spin_unlock_irq(&wdata->state.lock);
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
> +		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> +		wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> +		wiiproto_req_status(wdata);
> +	}
> +
>  
>  	ret = wiimote_cmd_wait_noint(wdata);
>  	if (ret)
>  		goto out_release;

[Severity: Medium]
Does this mix goto-based cleanup labels with scope-based cleanup?

The cleanup guidelines state that for a given routine, all resources requiring
cleanup should be converted to scope-based cleanup, or none of them should be.

This function (as well as wiimote_init_check() and wiimote_modules_load())
still uses goto-based cleanup like goto out_release; and goto error;
combined with the new scoped_guard() declarations.

[ ... ]
> @@ -1120,16 +1108,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -
> -	/* get state snapshot that we will then work on */
> -	flags = wdata->state.flags;
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
>  
> -	/* disable event forwarding temporarily */
> -	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
> -	wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
> +	    /* get state snapshot that we will then work on */
> +	    flags = wdata->state.flags;

[Severity: Low]
this isn't a bug, but it looks like the contents of this scoped_guard block 
are also indented with 4 spaces instead of a tab.

[ ... ]
> @@ -1774,14 +1749,11 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>  
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
> -	unsigned long flags;
> -
>  	wiidebug_deinit(wdata);
>  
>  	/* prevent init_worker from being scheduled again */
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock)
> +		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
>  
>  	cancel_work_sync(&wdata->init_worker);
>  	timer_shutdown_sync(&wdata->timer);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
in wiimote_destroy() have a use-after-free race condition?

Looking at the rest of wiimote_destroy(), cancel_work_sync(&wdata->queue.worker)
is called before hid_hw_stop(wdata->hdev). If a HID interrupt arrives
immediately after the worker is cancelled:

HID core calls wiimote_hid_event() ->
    handler_status_K() ->
    wiiproto_req_drm() ->
    wiimote_queue()

wiimote_queue() then calls schedule_work(&wdata->queue.worker) without
checking if the device is exiting.

Since hid_hw_stop() hasn't been called yet, the new worker is queued. After
hid_hw_stop() and kfree(wdata) complete, the queued worker can execute and
dereference the freed wdata.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=2