Re: [PATCH] HID: wacom: stop hardware after post-start probe failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error path in `wacom_parse_and_register()` fails to cancel asynchronous works and timers, leading to a Use-After-Free when the `wacom` structure is freed.
- [High] Dismantling software state before the hardware is stopped on failure paths creates race conditions with active interrupt handlers, leading to a NULL pointer dereference.
- [High] Releasing driver resources before stopping HID hardware in `wacom_wireless_work()` leads to a Use-After-Free on incoming events.
--

commit 6396bf377f7e81b67088a1a9500d030c3776a230
Author: Myeonghun Pak <mhun512@gmail.com>

HID: wacom: stop hardware after post-start probe failures

This commit modifies wacom_parse_and_register() to route post-hid_hw_start()
failures through hid_hw_stop() before releasing driver resources.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a32320b351e3e..76dc833f5f2e9 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c

[ ... ]

> @@ -2456,16 +2456,16 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
>  
>  	error = wacom_register_inputs(wacom);
>  	if (error)
> -		goto fail;
> +		goto fail_hw_stop;

[Severity: High]
This is a pre-existing issue, but does setting the input pointers to NULL
before stopping the hardware create a race condition?

When wacom_register_inputs() fails, its error path sets wacom_wac->pad_input
to NULL. However, because hid_hw_stop() hasn't been called yet, the
hardware can still generate interrupts:

wacom_parse_and_register()
  wacom_register_inputs()
    fail:
      wacom_wac->pad_input = NULL;
      return error;
  goto fail_hw_stop;

If an interrupt arrives before hid_hw_stop() completes, will the interrupt
handler dereference the NULL pad_input pointer?

>  
>  	if (wacom->wacom_wac.features.device_type & WACOM_DEVICETYPE_PAD) {
>  		error = wacom_initialize_leds(wacom);
>  		if (error)
> -			goto fail;
> +			goto fail_hw_stop;
>  
>  		error = wacom_initialize_remotes(wacom);
>  		if (error)
> -			goto fail;
> +			goto fail_hw_stop;
>  	}
>  
>  	if (!wireless) {

[ ... ]

> @@ -2496,6 +2496,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
>  	return 0;
>  
>  fail_quirks:
> +fail_hw_stop:
>  	hid_hw_stop(hdev);

[Severity: High]
This is a pre-existing issue, but does this error path leave pending
asynchronous works and timers running?

While hid_hw_stop() stops new interrupts from the hardware, works like
wireless_work, battery_work, remote_work, or timers that were already
scheduled while the hardware was active might still be pending. 

Because wacom_probe() will propagate this error and trigger devres cleanup
to free the wacom structure, could these pending works execute later and
attempt to dereference the freed wacom pointer?

>  fail:
>  	wacom_release_resources(wacom);

[Severity: High]
This is a pre-existing issue, but does releasing resources before
stopping the hardware in other code paths create a similar race
condition?

For example, in wacom_wireless_work(), wacom_release_resources() is
called before stopping the HID hardware. This frees resources like
pen_fifo via devres_release_group().

If an incoming HID report arrives after devres_release_group() but
before the hardware is stopped, could wacom_raw_event() attempt to
push data to the freed pen_fifo via wacom_wac_queue_insert(), causing
a use-after-free?

>  	return error;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524175552.1973-1-mhun512@gmail.com?part=1