Re: [PATCH v2 1/2] HID: wacom: Fix Use-After-Free in wacom_intuos_pad

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Moving wacom_add_shared_data() to the end of wacom_parse_and_register() causes an unconditional NULL pointer dereference during device probe.
- [High] Moving wacom_add_shared_data() to the end of the probe function creates a window where wacom_wac->shared is NULL while hardware interrupts are active, leading to an asynchronous NULL pointer dereference.

Pre-existing issues:
- [High] The patch incompletely fixes a Time-of-Check to Time-of-Use (TOCTOU) race condition. It successfully converts wacom_intuos_pad() to use RCU, but leaves identical lockless accesses unprotected in wacom_wac_pad_event(), wacom_wireless_irq(), and wacom_status_irq().
- [High] Modifying input capabilities on the sibling touch device after it has already been registered with the input core.
--

commit 5bec6e8c12bc84f46d120679cc4bef944e215dc2
Author: Lee Jones <lee@kernel.org>

HID: wacom: Fix Use-After-Free in wacom_intuos_pad
    
This commit resolves a Time-of-Check to Time-of-Use race condition by
introducing RCU protection for the touch_input pointer. It also delays the
call to wacom_add_shared_data() to ensure correct devres release order.

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

[ ... ]

> @@ -2362,11 +2374,14 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
>  	}
>  
>  	if (wacom_wac->shared->has_mute_touch_switch &&
> -	    wacom_wac->shared->touch_input) {
> -		set_bit(EV_SW, wacom_wac->shared->touch_input->evbit);
> -		input_set_capability(wacom_wac->shared->touch_input, EV_SW,
> -				     SW_MUTE_DEVICE);
> +	    rcu_access_pointer(wacom_wac->shared->touch_input)) {
> +		struct input_dev *touch_input = rcu_dereference_protected(wacom_wac->shared->touch_input,
> +									lockdep_is_held(&wacom_udev_list_lock));
> +		set_bit(EV_SW, touch_input->evbit);
> +		input_set_capability(touch_input, EV_SW, SW_MUTE_DEVICE);
>  	}
> +
> +	mutex_unlock(&wacom_udev_list_lock);
>  }

[Severity: High]
This is a pre-existing issue, but does this capability modification happen
too late?

In wacom_set_shared_values(), input capabilities are modified on the sibling
touch device. Because the Pen interface probes after the Touch interface,
this occurs on a sibling's touch_input device that was already fully probed
and registered.

The input subsystem requires that all capabilities must be fully initialized
before calling input_register_device(). Modifying capabilities
post-registration means the ADD uevent has already been emitted, and
userspace components miss the capability changes and might fail to process
events like SW_MUTE_DEVICE.

[ ... ]

> @@ -2442,10 +2457,6 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
>  		goto fail;
>  	}
>  
> -	error = wacom_add_shared_data(hdev);
> -	if (error)
> -		goto fail;
> -
>  	error = wacom_setup_inputs(wacom);
>  	if (error)
>  		goto fail;

[Severity: Critical]
Does this change introduce a regression during device probe?

By removing wacom_add_shared_data() from this earlier location,
wacom_wac->shared is left uninitialized (NULL). However, wacom_setup_inputs()
is called immediately after:

wacom_setup_inputs()
   wacom_setup_touch_input_capabilities()
       if (wacom_wac->shared->touch->product == 0x361) {

Because wacom_wac->shared is unconditionally dereferenced for some devices
like INTUOSP2_BT and WACOM_27QHDT inside wacom_setup_touch_input_capabilities(),
could this cause a NULL pointer dereference during initialization?

[ ... ]

> @@ -2496,6 +2507,10 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
>  		}
>  	}
>  
> +	error = wacom_add_shared_data(hdev);
> +	if (error)
> +		goto fail_quirks;
> +
>  	wacom_set_shared_values(wacom_wac);
>  	devres_close_group(&hdev->dev, wacom);

[Severity: High]
Is there a race condition here since wacom_add_shared_data() is now called
at the very end of the probe function?

Before reaching this point, wacom_parse_and_register() has already called
hid_hw_start() and wacom_register_inputs(). hid_hw_start() enables incoming
HID reports and wacom_register_inputs() exposes the device to userspace.

If a hardware interrupt fires right after hid_hw_start(), the interrupt
handlers can execute while wacom_wac->shared is still NULL:

wacom_bpt_pen()
    wacom->shared->stylus_in_proximity = true;

Could this cause an asynchronous NULL pointer dereference if a report
arrives before the shared data is allocated?

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

[ ... ]

> @@ -650,11 +650,18 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
>  	input_report_key(input, KEY_CONTROLPANEL, menu);
>  	input_report_key(input, KEY_INFO, info);
>  
> -	if (wacom->shared && wacom->shared->touch_input) {
> -		input_report_switch(wacom->shared->touch_input,
> -				    SW_MUTE_DEVICE,
> -				    !wacom->shared->is_touch_on);
> -		input_sync(wacom->shared->touch_input);
> +	if (wacom->shared) {
> +		struct input_dev *touch_input;
> +
> +		rcu_read_lock();
> +		touch_input = rcu_dereference(wacom->shared->touch_input);
> +		if (touch_input) {
> +			input_report_switch(touch_input,
> +					    SW_MUTE_DEVICE,
> +					    !wacom->shared->is_touch_on);
> +			input_sync(touch_input);
> +		}
> +		rcu_read_unlock();
>  	}

[Severity: High]
This is a pre-existing issue, but did this patch miss a few other places
where touch_input is accessed locklessly?

This update successfully converts wacom_intuos_pad() to use RCU, but there
are other HID event handlers in wacom_wac.c that still directly dereference
wacom_wac->shared->touch_input without holding rcu_read_lock().

For example, in wacom_wac_pad_event():

    if (wacom_wac->shared->touch_input) {
        ...
        input_report_switch(wacom_wac->shared->touch_input, ...);

Similar direct accesses happen in wacom_wireless_irq() and
wacom_status_irq(). Because these readers do not establish an RCU read-side
critical section, synchronize_rcu() in wacom_remove_shared_data() will not
wait for them.

If the sibling touch device is unbound, could the input_dev be freed while
these handlers are still running, resulting in the same use-after-free issue?
Direct access to an __rcu pointer may also trigger Sparse warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601140845.1237227-1-lee@kernel.org?part=1