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

[Lee Jones <lee@kernel.org>]

On Tue, 25 Aug 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Infinite probe deferral if sibling interface is a Pad or unsupported HID device
> - [Medium] TOCTOU race condition in shared capabilities advertisement

Looking into this now.

It's frustrating that my local Sashiko didn't flag this.

> Pre-existing issues:
> - [High] Use-After-Free of devm-allocated driver data on probe error path
> --
> 
> commit 4eb131c54fac495d482614e8b0c028a0f23f19f7
> Author: Lee Jones <lee@kernel.org>
> 
> HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration
> 
> This patch moves the SW_MUTE_DEVICE capability setup before input device
> registration to comply with input subsystem guidelines. For generic touch
> devices relying on a sibling Pen or Pad interface, it defers registration
> via -EPROBE_DEFER until the shared capabilities are initialized.
> 
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 0eafa483b7f75..2738d4f515e6a 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> 
> [ ... ]
> 
> > @@ -2358,13 +2358,44 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
> >  		if (wacom_wac->is_soft_touch_switch)
> >  			wacom_wac->shared->is_touch_on = true;
> >  	}
> > +}
> > +
> > +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))
> > +		return false;
> > +
> > +	if (!hid_is_usb(hdev) || !wacom->usbdev)
> > +		return false;
> >  
> > -	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);
> > +	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)
> > +				return true;
> > +		}
> >  	}
> > +
> > +	return false;
> >  }
> 
> [Severity: High]
> Could this lead to infinite probe deferral if the sibling interface is a Pad
> or an unsupported HID device?
> 
> If a composite Wacom device presents a generic Touch interface and a Pad
> interface (without a Pen), or a generic HID interface not handled by this
> driver, this loop will return true indefinitely. The logic requires
> shared->pen to be populated to stop deferring, but wacom_add_shared_data()
> only populates shared->pen for Pen interfaces, leaving the Touch device
> unusable.
> 
> >  static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> > @@ -2444,6 +2475,11 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> >  	if (error)
> >  		goto fail;
> >  
> > +	if (wacom_sibling_pending(wacom)) {
> > +		error = -EPROBE_DEFER;
> > +		goto fail;
> > +	}
> > +
> >  	error = wacom_setup_inputs(wacom);
> 
> [Severity: High]
> This is a pre-existing issue, but does the error path in this function fail
> to cancel scheduled asynchronous works, leading to a Use-After-Free?
> 
> If wacom_query_tablet_data() schedules init_work with a delay, and a
> subsequent step like hid_hw_open() fails:
> 
> wacom_parse_and_register() {
>     ...
>     if (!wireless) {
>         wacom_query_tablet_data(wacom); /* schedules init_work */
>     }
>     ...
>     if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
>         error = hid_hw_open(hdev);
>         if (error) {
>             goto fail_hw_stop;
>         }
>     }
>     ...
> fail_hw_stop:
>     hid_hw_stop(hdev);
> fail:
>     wacom_release_resources(wacom);
>     return error;
> }
> 
> The error path returns and the driver core devres cleanup frees the
> struct wacom. When the delayed work later executes, it will access the
> freed memory. Should the error path explicitly cancel pending works?
> 
> > 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) {
> > +			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: Medium]
> Is there a Time-of-Check to Time-of-Use (TOCTOU) race condition here when
> enumerating concurrent USB interfaces?
> 
> In wacom_add_shared_data(), the Pen interface publishes itself to
> shared->pen early. If the Touch device probe concurrently reads shared->pen
> in wacom_sibling_pending(), it will break out of its deferral loop. However,
> it may reach this point and read shared->has_mute_touch_switch before the
> Pen device has finished parsing its descriptors and written to it in
> wacom_set_shared_values(), causing the Touch device to miss advertising
> the capability.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260825151242.576456-1-lee@kernel.org?part=2

-- 
Lee Jones