Re: [PATCH v9 2/5] HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Generic touch devices will enter an infinite -EPROBE_DEFER loop if they share a USB device with a non-Pen/Pad HID interface.
- [High] Standalone generic touch devices with a hardware mute switch will no longer advertise the SW_MUTE_DEVICE capability.
- [Medium] Missing memory barrier / RCU acquire semantics when reading shared sibling state causes a TOCTOU race.

Pre-existing issues:
- [High] Missing cancel_delayed_work_sync in the hid_hw_open error path causes a Use-After-Free.
--

commit bd90131f0064218ea089e2069fff71db08186576
Author: Lee Jones <lee@kernel.org>
Subject: HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration

This commit moves the SW_MUTE_DEVICE capability setup to
wacom_setup_touch_input_capabilities() for touch devices. It also defers
registration for generic touch devices until their sibling Pen/Pad interface
has been parsed and shared capabilities are initialized.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 0eafa483b7f75..026a6be467d3d 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2359,12 +2348,52 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
[ ... ]
> +static bool wacom_sibling_pending(struct wacom *wacom)
> +{
> +	struct hid_device *hdev = wacom->hdev;
> +	const struct wacom_features *features = &wacom->wacom_wac.features;
> +	struct usb_host_config *actconfig;
> +	int i;
> +
> +	if (features->type != HID_GENERIC ||
> +	    !(features->device_type & WACOM_DEVICETYPE_TOUCH))
> +		return false;
> +
> +	if (wacom->wacom_wac.shared &&
> +	    rcu_access_pointer(wacom->wacom_wac.shared->pen))

[Severity: Medium]
Is there a potential TOCTOU race here due to a missing acquire barrier?

Because rcu_access_pointer() does not provide an acquire memory barrier,
could weakly ordered CPUs reorder the reads, seeing the updated pen pointer
here but a stale false value for the switch flag later when
wacom_setup_touch_input_capabilities() checks shared->has_mute_touch_switch?

> +		return false;
> +
> +	if (!hid_is_usb(hdev) || !wacom->usbdev)
> +		return false;
> +
> +	if (features->oPid != HID_ANY_ID && features->oPid != 0)
> +		return true;
> +
> +	actconfig = wacom->usbdev->actconfig;
> +	if (actconfig && actconfig->desc.bNumInterfaces > 1) {
> +		for (i = 0; i < actconfig->desc.bNumInterfaces; i++) {
> +			struct usb_interface *sibling_intf = actconfig->interface[i];
> +
> +			if (!sibling_intf || sibling_intf == wacom->intf)
> +				continue;
> +
> +			if (sibling_intf->cur_altsetting->desc.bInterfaceClass ==
> +			    USB_INTERFACE_CLASS_HID)

[Severity: High]
Could this overly broad check lead to an infinite -EPROBE_DEFER loop?

If a generic touch device shares a USB device with a standard HID interface
(like a keyboard or raw vendor HID), the Wacom driver might reject the
standard interface, causing hid-generic to bind to it instead.

Since hid-generic would not populate wacom_wac.shared->pen, this check
would repeatedly observe shared->pen == NULL and a sibling interface with
USB_INTERFACE_CLASS_HID, returning true indefinitely. It seems this could
leave the touch interface permanently trapped in probe deferral.

> +				return true;
> +		}
>  	}
> +
> +	return false;
>  }
[ ... ]
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 4436faf2d2bbd..a7e3817aa2c4f 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -3965,6 +3965,10 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  
>  	if (features->type == HID_GENERIC) {
>  		hid_dbg(hdev, "generic touch setup\n");
> +		if (wacom_wac->shared && wacom_wac->shared->has_mute_touch_switch) {

[Severity: High]
For standalone generic touch devices with a hardware mute switch, does this
result in the loss of the SW_MUTE_DEVICE capability?

The local parser sets the wacom_wac->has_mute_touch_switch flag, but this
now only checks the shared flag (wacom_wac->shared->has_mute_touch_switch).

However, wacom_set_shared_values() syncs the local flag to the shared
struct at the very end of wacom_parse_and_register(), which happens
after capabilities are set up. Could this leave the shared flag false
during setup for standalone devices, breaking userspace expectations?

> +			input_set_capability(input_dev, EV_SW, SW_MUTE_DEVICE);
> +			wacom_wac->has_mute_touch_switch = true;
> +		}
>  		/* setup has already been done */
>  		return 0;
>  	}

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
in the hid_hw_open() error path of wacom_parse_and_register().

During device probe, wacom_query_tablet_data() schedules wacom->init_work
with a 1-second delay. If the device has WACOM_DEVICETYPE_WL_MONITOR and
the subsequent hid_hw_open() fails:

	if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
		error = hid_hw_open(hdev);
		if (error) {
			hid_err(hdev, "hw open failed\n");
			goto fail_hw_stop;
		}
	}

The code jumps to fail_hw_stop without canceling the delayed work. Since
the devres framework frees the wacom structure on probe failure, does this
leave a dangling timer that will attempt to access freed memory when
wacom->init_work runs? Should cancel_delayed_work_sync(&wacom->init_work)
be called in this error path?

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