[PATCH 1/5] HID: wacom: Use hdev->product in wacom_setup_touch_input_capabilities

[PATCH 1/5] HID: wacom: Use hdev->product in wacom_setup_touch_input_capabilities

[Lee Jones]

Replace the lookup-dependent 'wacom_wac->shared->touch->product' references
with 'hdev->product' inside wacom_setup_touch_input_capabilities() since
'hdev' is already available (via container_of) and represents the touch
device itself.

Signed-off-by: Lee Jones <lee@kernel.org>
---

v7 -> v8: New patch

 drivers/hid/wacom_wac.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index a29bf051ada7..4436faf2d2bb 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3953,8 +3953,8 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 					 struct wacom_wac *wacom_wac)
 {
+	struct hid_device *hdev = container_of(wacom_wac, struct wacom, wacom_wac)->hdev;
 	struct wacom_features *features = &wacom_wac->features;
-
 	if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
 		return -ENODEV;
 
@@ -3963,9 +3963,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	else
 		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 
-	if (features->type == HID_GENERIC)
+	if (features->type == HID_GENERIC) {
+		hid_dbg(hdev, "generic touch setup\n");
 		/* setup has already been done */
 		return 0;
+	}
 
 	input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
 	__set_bit(BTN_TOUCH, input_dev->keybit);
@@ -3997,19 +3999,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 		input_dev->evbit[0] |= BIT_MASK(EV_SW);
 		__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
 
-		if (wacom_wac->shared->touch->product == 0x361) {
+		if (hdev->product == 0x361) {
 			input_set_abs_params(input_dev, ABS_MT_POSITION_X,
 					     0, 12440, 4, 0);
 			input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
 					     0, 8640, 4, 0);
-		}
-		else if (wacom_wac->shared->touch->product == 0x360) {
+		} else if (hdev->product == 0x360) {
 			input_set_abs_params(input_dev, ABS_MT_POSITION_X,
 					     0, 8960, 4, 0);
 			input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
 					     0, 5920, 4, 0);
-		}
-		else if (wacom_wac->shared->touch->product == 0x393) {
+		} else if (hdev->product == 0x393) {
 			input_set_abs_params(input_dev, ABS_MT_POSITION_X,
 					     0, 6400, 4, 0);
 			input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
@@ -4039,8 +4039,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 		fallthrough;
 
 	case WACOM_27QHDT:
-		if (wacom_wac->shared->touch->product == 0x32C ||
-		    wacom_wac->shared->touch->product == 0xF6) {
+		if (hdev->product == 0x32C ||
+		    hdev->product == 0xF6) {
 			input_dev->evbit[0] |= BIT_MASK(EV_SW);
 			__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
 			wacom_wac->has_mute_touch_switch = true;
-- 
2.55.0.887.g758fc8c411-goog

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

[Lee Jones]

Input subsystem guidelines require that device capabilities are advertised
before the input device is registered.  The Wacom driver was violating
this by advertising the SW_MUTE_DEVICE capability post-registration in
wacom_set_shared_values() (and duplicating it in device-specific setup
cases).

Resolve this by moving the SW_MUTE_DEVICE capability setup to
wacom_setup_touch_input_capabilities() for all touch devices that support
it.

For generic touch devices whose capabilities depend on mute switch
usages parsed from a sibling Pen/Pad interface, defer registration
with -EPROBE_DEFER until the sibling has parsed its descriptors and
initialized shared capabilities.

Fixes: d2ec58aee8b1 ("HID: wacom: generic: support generic touch switch")
Signed-off-by: Lee Jones <lee@kernel.org>
---

v4 -> v5: New patch used to split out SW_MUTE_DEVICE as per Jason's request
v5 -> v6: Unconditionally advertise SW_MUTE_DEVICE on generic touch devices
v6 -> v7: Only advertise SW_MUTE_DEVICE on composite USB generic touch devices
v7 -> v8: Replace heuristic with probe deferral until sibling Pen/Pad is parsed
          Split out 'hdev->product' cleanups into a separate standalone patch

 drivers/hid/wacom_sys.c | 46 ++++++++++++++++++++++++++++++++++++-----
 drivers/hid/wacom_wac.c |  4 ++++
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 0eafa483b7f7..2738d4f515e6 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;
 	}
+}
 
-	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);
+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 (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;
 }
 
 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);
 	if (error)
 		goto fail;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 4436faf2d2bb..a7e3817aa2c4 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;
 	}
-- 
2.55.0.887.g758fc8c411-goog

[PATCH 3/5] HID: wacom: Fix Use-After-Free in wacom_intuos_pad

[Lee Jones]

wacom_intuos_pad() accesses wacom->shared->touch_input locklessly
inside the interrupt handler context.  If the Touch sibling device
is disconnected, wacom_remove_shared_data() clears 'touch_input'
outside any lock, creating a Time-of-Check to Time-of-Use (TOCTOU)
race condition where a preempted reader in interrupt context
dereferences the freed pointer, leading to a Use-After-Free.

Resolve this by introducing RCU protection for the touch_input
pointer:

 - Annotate 'touch_input' in wacom_shared struct with __rcu
 - Wrap all lockless readers in wacom_wac.c with guard(rcu)() and
   rcu_dereference() using a unified wacom_report_touch_mute()
   helper
 - Update writers in wacom_sys.c using rcu_assign_pointer()
 - Call synchronize_rcu() in wacom_remove_shared_data() to ensure
   all active RCU readers have finished before the input device is
   freed

Also wrap wacom_set_shared_values() and touch/pen assignments in
wacom_add_shared_data() inside the wacom_udev_list_lock to serialize
concurrent probe assignments, and verify that 'shared->touch == hdev'
before setting touch_input to prevent concurrent sibling probe state
desynchronization.

Fixes: 961794a00eab ("Input: wacom - add reporting of SW_MUTE_DEVICE events")
Signed-off-by: Lee Jones <lee@kernel.org>
---

v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
v3 -> v4: Dmitry's review [redundant check and guard()]
v4 -> v5: Jason's review [split, remove "awkward if"]
v5 -> v6: No change
v6 -> v7: No change
v7 -> v8: No functional change - cleaned up a stray whitespace/blank line deletion hunk

 drivers/hid/wacom_sys.c | 34 +++++++++++++++++++++++-----------
 drivers/hid/wacom_wac.c | 36 ++++++++++++++++++------------------
 drivers/hid/wacom_wac.h |  2 +-
 3 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2738d4f515e6..b88726cbbb4e 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -875,10 +875,16 @@ static void wacom_remove_shared_data(void *res)
 		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
 				    shared);
 
-		if (wacom_wac->shared->touch == wacom->hdev)
-			wacom_wac->shared->touch = NULL;
-		else if (wacom_wac->shared->pen == wacom->hdev)
-			wacom_wac->shared->pen = NULL;
+		scoped_guard(mutex, &wacom_udev_list_lock) {
+			if (wacom_wac->shared->touch == wacom->hdev) {
+				wacom_wac->shared->touch = NULL;
+				rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
+			} else if (wacom_wac->shared->pen == wacom->hdev) {
+				wacom_wac->shared->pen = NULL;
+			}
+		}
+
+		synchronize_rcu();
 
 		kref_put(&data->kref, wacom_release_shared_data);
 		wacom_wac->shared = NULL;
@@ -907,6 +913,11 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 		list_add_tail(&data->list, &wacom_udev_list);
 	}
 
+	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
+		data->shared.touch = hdev;
+	else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
+		data->shared.pen = hdev;
+
 	mutex_unlock(&wacom_udev_list_lock);
 
 	wacom_wac->shared = &data->shared;
@@ -915,11 +926,6 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 	if (retval)
 		return retval;
 
-	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
-		wacom_wac->shared->touch = hdev;
-	else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
-		wacom_wac->shared->pen = hdev;
-
 	return retval;
 }
 
@@ -2343,9 +2349,15 @@ static void wacom_release_resources(struct wacom *wacom)
 
 static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
 {
+	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+
+	guard(mutex)(&wacom_udev_list_lock);
+
 	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
-		wacom_wac->shared->type = wacom_wac->features.type;
-		wacom_wac->shared->touch_input = wacom_wac->touch_input;
+		if (wacom_wac->shared->touch == wacom->hdev) {
+			wacom_wac->shared->type = wacom_wac->features.type;
+			rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
+		}
 	}
 
 	if (wacom_wac->has_mute_touch_switch) {
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index a7e3817aa2c4..f953851ad6bc 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -510,6 +510,18 @@ static void wacom_intuos_schedule_prox_event(struct wacom_wac *wacom_wac)
 	}
 }
 
+static void wacom_report_touch_mute(struct wacom_wac *wacom_wac, bool mute)
+{
+	struct input_dev *touch_input;
+
+	guard(rcu)();
+	touch_input = rcu_dereference(wacom_wac->shared->touch_input);
+	if (touch_input) {
+		input_report_switch(touch_input, SW_MUTE_DEVICE, mute);
+		input_sync(touch_input);
+	}
+}
+
 static int wacom_intuos_pad(struct wacom_wac *wacom)
 {
 	struct wacom_features *features = &wacom->features;
@@ -650,12 +662,8 @@ 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)
+		wacom_report_touch_mute(wacom, !wacom->shared->is_touch_on);
 
 	input_report_abs(input, ABS_RX, strip1);
 	input_report_abs(input, ABS_RY, strip2);
@@ -2155,7 +2163,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 	 */
 	if ((equivalent_usage == WACOM_HID_WD_MUTE_DEVICE) ||
 	   (equivalent_usage == WACOM_HID_WD_TOUCHONOFF)) {
-		if (wacom_wac->shared->touch_input) {
+		if (wacom_wac->shared) {
 			bool *is_touch_on = &wacom_wac->shared->is_touch_on;
 
 			if (equivalent_usage == WACOM_HID_WD_MUTE_DEVICE && value)
@@ -2163,9 +2171,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 			else if (equivalent_usage == WACOM_HID_WD_TOUCHONOFF)
 				*is_touch_on = value;
 
-			input_report_switch(wacom_wac->shared->touch_input,
-					    SW_MUTE_DEVICE, !(*is_touch_on));
-			input_sync(wacom_wac->shared->touch_input);
+			wacom_report_touch_mute(wacom_wac, !(*is_touch_on));
 		}
 		return;
 	}
@@ -3383,11 +3389,8 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 
 		if ((wacom->shared->type == INTUOSHT ||
 		    wacom->shared->type == INTUOSHT2) &&
-		    wacom->shared->touch_input &&
 		    wacom->shared->touch_max) {
-			input_report_switch(wacom->shared->touch_input,
-					SW_MUTE_DEVICE, data[5] & 0x40);
-			input_sync(wacom->shared->touch_input);
+			wacom_report_touch_mute(wacom, data[5] & 0x40);
 		}
 
 		pid = get_unaligned_be16(&data[6]);
@@ -3422,11 +3425,8 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
 
 	if ((features->type == INTUOSHT ||
 	    features->type == INTUOSHT2) &&
-	    wacom_wac->shared->touch_input &&
 	    features->touch_max) {
-		input_report_switch(wacom_wac->shared->touch_input,
-				    SW_MUTE_DEVICE, data[8] & 0x40);
-		input_sync(wacom_wac->shared->touch_input);
+		wacom_report_touch_mute(wacom_wac, data[8] & 0x40);
 	}
 
 	if (data[9] & 0x02) { /* wireless module is attached */
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 126bec6e5c0c..a8bbba4a6f37 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -285,7 +285,7 @@ struct wacom_shared {
 	/* for wireless device to access USB interfaces */
 	unsigned touch_max;
 	int type;
-	struct input_dev *touch_input;
+	struct input_dev __rcu *touch_input;
 	struct hid_device *pen;
 	struct hid_device *touch;
 	bool has_mute_touch_switch;
-- 
2.55.0.887.g758fc8c411-goog

[PATCH 4/5] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad

[Lee Jones]

wacom_bamboo_pad_pen_event() accesses wacom->shared->pen locklessly
relative to wacom_remove_shared_data() which nullifies it.  This
can lead to a Use-After-Free if the sibling device is removed while
events are being processed.

Resolve this by introducing RCU protection for pen and touch
pointers:

 - Annotate 'pen' and 'touch' in wacom_shared struct with __rcu.
 - Wrap lockless readers in wacom_bamboo_pad_pen_event() with
   rcu_read_lock() and rcu_dereference().
 - Update writers in wacom_sys.c using rcu_assign_pointer().
 - Use rcu_dereference_protected for comparisons under
   wacom_udev_list_lock.
 - Also use rcu_access_pointer in wacom_mode_change_work() to avoid
   warnings (while lockless access there remains a pre-existing issue).

Fixes: 8c97a765467c ("HID: wacom: add full support of the Wacom Bamboo PAD")
Signed-off-by: Lee Jones <lee@kernel.org>
---

v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
v3 -> v4: Dmitry's review [guard()]
v4 -> v5: No change
v5 -> v6: No change
v6 -> v7: No change
v7 -> v8: No change

 drivers/hid/wacom_sys.c | 36 +++++++++++++++++++++++++-----------
 drivers/hid/wacom_wac.c | 13 ++++++-------
 drivers/hid/wacom_wac.h |  4 ++--
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index b88726cbbb4e..79fd397e044a 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -876,11 +876,18 @@ static void wacom_remove_shared_data(void *res)
 				    shared);
 
 		scoped_guard(mutex, &wacom_udev_list_lock) {
-			if (wacom_wac->shared->touch == wacom->hdev) {
-				wacom_wac->shared->touch = NULL;
+			struct hid_device *touch =
+				rcu_dereference_protected(wacom_wac->shared->touch,
+							  lockdep_is_held(&wacom_udev_list_lock));
+			struct hid_device *pen =
+				rcu_dereference_protected(wacom_wac->shared->pen,
+							  lockdep_is_held(&wacom_udev_list_lock));
+
+			if (touch == wacom->hdev) {
+				rcu_assign_pointer(wacom_wac->shared->touch, NULL);
 				rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
-			} else if (wacom_wac->shared->pen == wacom->hdev) {
-				wacom_wac->shared->pen = NULL;
+			} else if (pen == wacom->hdev) {
+				rcu_assign_pointer(wacom_wac->shared->pen, NULL);
 			}
 		}
 
@@ -914,9 +921,9 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 	}
 
 	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
-		data->shared.touch = hdev;
+		rcu_assign_pointer(data->shared.touch, hdev);
 	else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
-		data->shared.pen = hdev;
+		rcu_assign_pointer(data->shared.pen, hdev);
 
 	mutex_unlock(&wacom_udev_list_lock);
 
@@ -2354,7 +2361,11 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
 	guard(mutex)(&wacom_udev_list_lock);
 
 	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
-		if (wacom_wac->shared->touch == wacom->hdev) {
+		struct hid_device *touch =
+			rcu_dereference_protected(wacom_wac->shared->touch,
+						  lockdep_is_held(&wacom_udev_list_lock));
+
+		if (touch == wacom->hdev) {
 			wacom_wac->shared->type = wacom_wac->features.type;
 			rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
 		}
@@ -2836,16 +2847,19 @@ static void wacom_mode_change_work(struct work_struct *work)
 	bool is_direct = wacom->wacom_wac.is_direct_mode;
 	int error = 0;
 
-	if (shared->pen) {
-		wacom1 = hid_get_drvdata(shared->pen);
+	struct hid_device *pen = rcu_access_pointer(shared->pen);
+	struct hid_device *touch = rcu_access_pointer(shared->touch);
+
+	if (pen) {
+		wacom1 = hid_get_drvdata(pen);
 		wacom_release_resources(wacom1);
 		hid_hw_stop(wacom1->hdev);
 		wacom1->wacom_wac.has_mode_change = true;
 		wacom1->wacom_wac.is_direct_mode = is_direct;
 	}
 
-	if (shared->touch) {
-		wacom2 = hid_get_drvdata(shared->touch);
+	if (touch) {
+		wacom2 = hid_get_drvdata(touch);
 		wacom_release_resources(wacom2);
 		hid_hw_stop(wacom2->hdev);
 		wacom2->wacom_wac.has_mode_change = true;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index f953851ad6bc..884d1cf7401c 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3294,6 +3294,7 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, size_t len)
 static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
 		unsigned char *data)
 {
+	struct hid_device *pen;
 	unsigned char prefix;
 
 	/*
@@ -3306,13 +3307,11 @@ static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
 	prefix = data[0];
 	data[0] = WACOM_REPORT_BPAD_PEN;
 
-	/*
-	 * actually reroute the event.
-	 * No need to check if wacom->shared->pen is valid, hid_input_report()
-	 * will check for us.
-	 */
-	hid_input_report(wacom->shared->pen, HID_INPUT_REPORT, data,
-			 WACOM_PKGLEN_PENABLED, 1);
+	guard(rcu)();
+	pen = rcu_dereference(wacom->shared->pen);
+	if (pen)
+		hid_input_report(pen, HID_INPUT_REPORT, data,
+				 WACOM_PKGLEN_PENABLED, 1);
 
 	data[0] = prefix;
 }
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index a8bbba4a6f37..170d6adbe02a 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -286,8 +286,8 @@ struct wacom_shared {
 	unsigned touch_max;
 	int type;
 	struct input_dev __rcu *touch_input;
-	struct hid_device *pen;
-	struct hid_device *touch;
+	struct hid_device __rcu *pen;
+	struct hid_device __rcu *touch;
 	bool has_mute_touch_switch;
 	bool is_touch_on;
 };
-- 
2.55.0.887.g758fc8c411-goog

[PATCH 5/5] HID: wacom: Redesign shared sibling data lifecycle

[Lee Jones]

The Wacom driver coordinates state between sibling interfaces of
the same physical device using a shared structure 'wacom_shared'
inside 'wacom_hdev_data'.  The driver kept a volatile representative
pointer 'data->dev' pointing to a sibling 'hid_device' for physical
path comparisons during sibling matching.

This pointer management is fragile.  When the representative device
is disconnected, wacom_remove_shared_data() failed to clear/update
'data->dev', leading to a Use-After-Free vulnerability when
subsequent sibling probes dereference the dangling 'data->dev'
pointer.

Resolve this issue by redesigning the sibling data lifecycle:

 - Eliminate the volatile 'data->dev' representative pointer
   completely
 - Redesign 'wacom_hdev_data' to store stable static copies of the
   required attributes upon first allocation: 'phys' path string,
   'vendor', 'product' IDs and the sibling's 'device_type'
 - Use these static attributes for stable sibling matching in
   wacom_are_sibling() and wacom_get_hdev_data()

This ensures sibling matching remains safe and stable even if
individual siblings are dynamically added or removed.

To secure the lifecycle against concurrent probe/disconnect races:

 - Switch kref_put() to kref_put_mutex() in
   wacom_remove_shared_data() to serialize refcount drops with list
   traversal and lookup
 - Modify wacom_release_shared_data() to assume the list lock is
   already held

Also, do not accumulate the 'device_type' capability flag during
subsequent sibling probes.  Keeping only the first probed sibling's
device_type exactly preserves the original sibling matching behavior
without introducing side effects.

Fixes: 4492efffffeb ("Input: wacom - share pen info with touch of the same ID")
Signed-off-by: Lee Jones <lee@kernel.org>
---

v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
v3 -> v4: No change
v4 -> v5: No change
v5 -> v6: No change
v6 -> v7: No change
v7 -> v8: No change

drivers/hid/wacom_sys.c | 70 +++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 79fd397e044a..d4b6dc9eb0c8 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -757,27 +757,47 @@ static void wacom_retrieve_hid_descriptor(struct hid_device *hdev,
 struct wacom_hdev_data {
 	struct list_head list;
 	struct kref kref;
-	struct hid_device *dev;
+	char phys[64];
+	__u32 vendor;
+	__u32 product;
+	__u32 device_type;
 	struct wacom_shared shared;
 };
 
+static bool wacom_compare_device_paths(struct hid_device *hdev_a,
+				       const char *phys_b, char separator)
+{
+	const char *p1 = strrchr(hdev_a->phys, separator);
+	const char *p2 = strrchr(phys_b, separator);
+	int n1, n2;
+
+	if (!p1 || !p2)
+		return false;
+
+	n1 = p1 - hdev_a->phys;
+	n2 = p2 - phys_b;
+
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+		return false;
+
+	return !strncmp(hdev_a->phys, phys_b, n1);
+}
+
 static LIST_HEAD(wacom_udev_list);
 static DEFINE_MUTEX(wacom_udev_list_lock);
 
 static bool wacom_are_sibling(struct hid_device *hdev,
-		struct hid_device *sibling)
+		struct wacom_hdev_data *data)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_features *features = &wacom->wacom_wac.features;
-	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
-	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
 	__u32 oVid = features->oVid ? features->oVid : hdev->vendor;
 	__u32 oPid = features->oPid ? features->oPid : hdev->product;
 
 	/* The defined oVid/oPid must match that of the sibling */
-	if (features->oVid != HID_ANY_ID && sibling->vendor != oVid)
+	if (features->oVid != HID_ANY_ID && data->vendor != oVid)
 		return false;
-	if (features->oPid != HID_ANY_ID && sibling->product != oPid)
+	if (features->oPid != HID_ANY_ID && data->product != oPid)
 		return false;
 
 	/*
@@ -785,11 +805,11 @@ static bool wacom_are_sibling(struct hid_device *hdev,
 	 * device path, while those with different VID/PID must share
 	 * the same physical parent device path.
 	 */
-	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
-		if (!hid_compare_device_paths(hdev, sibling, '/'))
+	if (hdev->vendor == data->vendor && hdev->product == data->product) {
+		if (!wacom_compare_device_paths(hdev, data->phys, '/'))
 			return false;
 	} else {
-		if (!hid_compare_device_paths(hdev, sibling, '.'))
+		if (!wacom_compare_device_paths(hdev, data->phys, '.'))
 			return false;
 	}
 
@@ -802,7 +822,7 @@ static bool wacom_are_sibling(struct hid_device *hdev,
 	 * devices.
 	 */
 	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
-	    !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+	    !(data->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
 	/*
@@ -810,17 +830,17 @@ static bool wacom_are_sibling(struct hid_device *hdev,
 	 * devices.
 	 */
 	if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
-	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+	    (data->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
 	/* Pen devices may only be siblings of touch devices */
 	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
-	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+	    !(data->device_type & WACOM_DEVICETYPE_TOUCH))
 		return false;
 
 	/* Touch devices may only be siblings of pen devices */
 	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
-	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
+	    !(data->device_type & WACOM_DEVICETYPE_PEN))
 		return false;
 
 	/*
@@ -836,7 +856,7 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 
 	/* Try to find an already-probed interface from the same device */
 	list_for_each_entry(data, &wacom_udev_list, list) {
-		if (hid_compare_device_paths(hdev, data->dev, '/')) {
+		if (wacom_compare_device_paths(hdev, data->phys, '/')) {
 			kref_get(&data->kref);
 			return data;
 		}
@@ -844,7 +864,7 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 
 	/* Fallback to finding devices that appear to be "siblings" */
 	list_for_each_entry(data, &wacom_udev_list, list) {
-		if (wacom_are_sibling(hdev, data->dev)) {
+		if (wacom_are_sibling(hdev, data)) {
 			kref_get(&data->kref);
 			return data;
 		}
@@ -858,18 +878,15 @@ static void wacom_release_shared_data(struct kref *kref)
 	struct wacom_hdev_data *data =
 		container_of(kref, struct wacom_hdev_data, kref);
 
-	mutex_lock(&wacom_udev_list_lock);
 	list_del(&data->list);
-	mutex_unlock(&wacom_udev_list_lock);
-
 	kfree(data);
 }
 
 static void wacom_remove_shared_data(void *res)
 {
-	struct wacom *wacom = res;
+	struct wacom *res_wacom = res;
 	struct wacom_hdev_data *data;
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_wac *wacom_wac = &res_wacom->wacom_wac;
 
 	if (wacom_wac->shared) {
 		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
@@ -883,17 +900,19 @@ static void wacom_remove_shared_data(void *res)
 				rcu_dereference_protected(wacom_wac->shared->pen,
 							  lockdep_is_held(&wacom_udev_list_lock));
 
-			if (touch == wacom->hdev) {
+			if (touch == res_wacom->hdev) {
 				rcu_assign_pointer(wacom_wac->shared->touch, NULL);
 				rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
-			} else if (pen == wacom->hdev) {
+			} else if (pen == res_wacom->hdev) {
 				rcu_assign_pointer(wacom_wac->shared->pen, NULL);
 			}
 		}
 
 		synchronize_rcu();
 
-		kref_put(&data->kref, wacom_release_shared_data);
+		if (kref_put_mutex(&data->kref, wacom_release_shared_data, &wacom_udev_list_lock))
+			mutex_unlock(&wacom_udev_list_lock);
+
 		wacom_wac->shared = NULL;
 	}
 }
@@ -916,7 +935,10 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 		}
 
 		kref_init(&data->kref);
-		data->dev = hdev;
+		strscpy(data->phys, hdev->phys, sizeof(data->phys));
+		data->vendor = hdev->vendor;
+		data->product = hdev->product;
+		data->device_type = wacom_wac->features.device_type;
 		list_add_tail(&data->list, &wacom_udev_list);
 	}
 
-- 
2.55.0.887.g758fc8c411-goog

Re: [PATCH 1/5] HID: wacom: Use hdev->product in wacom_setup_touch_input_capabilities

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cleanup of workqueues and timers in the probe error path leads to a Use-After-Free vulnerability.
--

commit 84f0c894069529e6d333664a987e925e1ef88eb8
Author: Lee Jones <lee@kernel.org>

HID: wacom: Use hdev->product in wacom_setup_touch_input_capabilities

This commit simplifies wacom_setup_touch_input_capabilities() by replacing
lookups of wacom_wac->shared->touch->product with direct access to
hdev->product. It retrieves the hdev pointer using container_of() on the
provided wacom_wac.

> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index a29bf051ada7c..4436faf2d2bbd 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
[ ... ]

While reviewing this patch, I noticed a pre-existing issue in
wacom_parse_and_register() located in drivers/hid/wacom_sys.c:

drivers/hid/wacom_sys.c:wacom_parse_and_register() {
    ...
	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;
		}
	}
    ...
fail_hw_stop:
	hid_hw_stop(hdev);
fail:
	wacom_release_resources(wacom);
	return error;
}

[Severity: High]
This isn't a regression introduced by this patch, but is a pre-existing
issue. Does this error path fail to cancel asynchronously scheduled works?

In wacom_probe(), the wacom structure is allocated via devm_kzalloc().
During wacom_parse_and_register(), hid_hw_start() is called which enables
hardware interrupts that can schedule async works (e.g., battery_work).
Additionally, wacom_query_tablet_data() explicitly schedules init_work.

If a subsequent initialization step fails (like hid_hw_open() above), the
function jumps to fail_hw_stop and returns an error without cancelling the
pending works.

Since wacom_probe() then returns this error, causing the devres cleanup to
free the wacom structure, could these pending works later execute and
access the freed memory?

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

Re: [PATCH 4/5] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless access to `shared->pen` and `shared->touch` in `wacom_mode_change_work` without RCU read lock leads to Use-After-Free.
- [High] Asynchronous resources started during probe are not cancelled if a subsequent probe step fails.
--

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

HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
    
This patch resolves a use-after-free issue in wacom_bamboo_pad_pen_event
by introducing RCU protection for the pen and touch pointers.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index b88726cbbb4ec..79fd397e044af 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2836,16 +2847,19 @@ static void wacom_mode_change_work(struct work_struct *work)
>  	bool is_direct = wacom->wacom_wac.is_direct_mode;
>  	int error = 0;
>  
> -	if (shared->pen) {
> -		wacom1 = hid_get_drvdata(shared->pen);
> +	struct hid_device *pen = rcu_access_pointer(shared->pen);
> +	struct hid_device *touch = rcu_access_pointer(shared->touch);
> +
> +	if (pen) {
> +		wacom1 = hid_get_drvdata(pen);
>  		wacom_release_resources(wacom1);
>  		hid_hw_stop(wacom1->hdev);

[Severity: High]
This is a pre-existing issue, but does using rcu_access_pointer() here leave
the pointer unprotected?

The rcu_access_pointer() helper doesn't establish an RCU read-side critical
section. This means a concurrent removal of the sibling device could call
wacom_remove_shared_data() and nullify the shared pointer.

If the pointer is nullified, synchronize_rcu() won't wait for this work item
to finish.

Could this sequence allow the device and its driver data to be freed while
we're still executing hid_hw_stop() on it, resulting in a use-after-free?

>  		wacom1->wacom_wac.has_mode_change = true;
>  		wacom1->wacom_wac.is_direct_mode = is_direct;
>  	}
[ ... ]

Also, regarding a pre-existing issue in wacom_parse_and_register() not modified
by this patch:

[Severity: High]
This isn't a bug introduced by this patch, but does the error path here fail to
cancel asynchronous works?

When an initialization step fails after hid_hw_start(hdev) is called,
the cleanup goes to fail_hw_stop:

fail_hw_stop:
	hid_hw_stop(hdev);
fail:
	wacom_release_resources(wacom);
	return error;

The call to hid_hw_start() enables interrupts, which could schedule work items
like wacom->battery_work, wacom->wireless_work, or wacom->mode_change_work.

Does failing to cancel these scheduled works before returning allow them to
execute after devres has freed the wacom structure?

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

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

[sashiko-bot]

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

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

Re: [PATCH 3/5] HID: wacom: Fix Use-After-Free in wacom_intuos_pad

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cancellation of delayed work on probe error path leads to Use-After-Free.
- [High] Lockless accesses to sibling pointers lead to TOCTOU Use-After-Free if a sibling device is disconnected concurrently.
--

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

    HID: wacom: Fix Use-After-Free in wacom_intuos_pad
    
    wacom_intuos_pad() accesses wacom->shared->touch_input locklessly
    inside the interrupt handler context, leading to a Use-After-Free if
    the sibling device is disconnected. This patch resolves the race by
    introducing RCU protection for the touch_input pointer.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2738d4f515e6a..b88726cbbb4ec 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -875,10 +875,16 @@ static void wacom_remove_shared_data(void *res)
>  		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
>  				    shared);
>  
> -		if (wacom_wac->shared->touch == wacom->hdev)
> -			wacom_wac->shared->touch = NULL;
> -		else if (wacom_wac->shared->pen == wacom->hdev)
> -			wacom_wac->shared->pen = NULL;
> +		scoped_guard(mutex, &wacom_udev_list_lock) {
> +			if (wacom_wac->shared->touch == wacom->hdev) {
> +				wacom_wac->shared->touch = NULL;
> +				rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
> +			} else if (wacom_wac->shared->pen == wacom->hdev) {
> +				wacom_wac->shared->pen = NULL;
> +			}
> +		}
> +
> +		synchronize_rcu();

[Severity: High]
This isn't a bug introduced by this patch, but does wacom_mode_change_work()
still have a TOCTOU Use-After-Free race with this function?

wacom_mode_change_work() accesses the shared->pen and shared->touch pointers
locklessly, dereferences them via hid_get_drvdata(), and then calls blocking
operations:

drivers/hid/wacom_sys.c:wacom_mode_change_work() {
    ...
	if (shared->pen) {
		wacom1 = hid_get_drvdata(shared->pen);
		wacom_release_resources(wacom1);
		hid_hw_stop(wacom1->hdev);
    ...
}

If a sibling device is concurrently disconnected, its wacom_remove_shared_data()
executes. The synchronize_rcu() added here does not protect process-context
readers in wacom_mode_change_work() that do not hold an RCU read lock.

The disconnected sibling's devres cleanup runs, freeing the sibling's memory,
while the concurrent wacom_mode_change_work() from the still-connected sibling
dereferences the freed pointer.

Can we safely access these pointers in wacom_mode_change_work() without holding
wacom_udev_list_lock or using another mechanism to prevent the sibling from
being freed?

[ ... ]

> @@ -2343,9 +2349,15 @@ static void wacom_release_resources(struct wacom *wacom)
>  
>  static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
>  {
> +	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
> +
> +	guard(mutex)(&wacom_udev_list_lock);
> +
>  	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
> -		wacom_wac->shared->type = wacom_wac->features.type;
> -		wacom_wac->shared->touch_input = wacom_wac->touch_input;
> +		if (wacom_wac->shared->touch == wacom->hdev) {
> +			wacom_wac->shared->type = wacom_wac->features.type;
> +			rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but does wacom_parse_and_register() leak the
wacom->init_work on its error path, potentially leading to a Use-After-Free?

During probe, wacom_query_tablet_data() schedules wacom->init_work with
a 1-second delay. If a subsequent initialization step fails (such as
hid_hw_open() returning an error), the code jumps to the error path, which calls
wacom_release_resources() but fails to cancel the delayed work:

drivers/hid/wacom_sys.c:wacom_parse_and_register() {
    ...
fail_hw_stop:
	hid_hw_stop(hdev);
fail:
	wacom_release_resources(wacom);
	return error;
}

The probe function will propagate the error and devres will automatically free
the wacom structure. When the timer expires, the delayed work will execute and
dereference the freed wacom structure.

Should we add a cancel_delayed_work_sync(&wacom->init_work) in this error path?

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

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

[Lee Jones]

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

Re: [PATCH 1/5] HID: wacom: Use hdev->product in wacom_setup_touch_input_capabilities

[Ping Cheng]

Hi Lee,

Thank you for your effort and persistence! We (Wacom and Wacom's
customers) are lucky to have people like you working in this
community!

In addition to making sure that you know your contribution is greatly
appreciated, I am giving you an initial testing feedback so you know
you are on the right track.

I manually tested a few older and new Wacom tablets. Some of them have
touch switches, some don't. They all worked well!

Two more details I'd like to share.

The first one is about the touch-only tablet. In all the models of
Wacom tablets, there was only one model that is touch-only. It was a
Bamboo Touch (model CTT-460) released on September 24, 2009. So, it
was more than 17 years ago. However, the quality of Wacom tablets are
so high, it is possible that some people are still using those ones
;).

The second one is about the type of touch on/off switches. There are
two types of touch on/off switches: it can be a soft key or hardware
switch. The hardware touch switch is easy to understand. The soft key
touch switch is actually an on-screen display in the shape of fingers,
where the touch on/off is controlled by the driver: [1]. This softkey
touch switch, somehow, is not reported to the userland by the existing
driver. Your patchset doesn't show it either.

I will do more testing to figure out the root cause of the softkey issue.

Cheers,
Ping

[1] ttps://github.com/linuxwacom/input-wacom/blob/master/4.18/wacom_wac.c#L2063

On Tue, Aug 25, 2026 at 10:09 AM Lee Jones <lee@kernel.org> wrote:
>
> Replace the lookup-dependent 'wacom_wac->shared->touch->product' references
> with 'hdev->product' inside wacom_setup_touch_input_capabilities() since
> 'hdev' is already available (via container_of) and represents the touch
> device itself.
>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>
> v7 -> v8: New patch
>
>  drivers/hid/wacom_wac.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index a29bf051ada7..4436faf2d2bb 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -3953,8 +3953,8 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>                                          struct wacom_wac *wacom_wac)
>  {
> +       struct hid_device *hdev = container_of(wacom_wac, struct wacom, wacom_wac)->hdev;
>         struct wacom_features *features = &wacom_wac->features;
> -
>         if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
>                 return -ENODEV;
>
> @@ -3963,9 +3963,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>         else
>                 __set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>
> -       if (features->type == HID_GENERIC)
> +       if (features->type == HID_GENERIC) {
> +               hid_dbg(hdev, "generic touch setup\n");
>                 /* setup has already been done */
>                 return 0;
> +       }
>
>         input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>         __set_bit(BTN_TOUCH, input_dev->keybit);
> @@ -3997,19 +3999,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>                 input_dev->evbit[0] |= BIT_MASK(EV_SW);
>                 __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
>
> -               if (wacom_wac->shared->touch->product == 0x361) {
> +               if (hdev->product == 0x361) {
>                         input_set_abs_params(input_dev, ABS_MT_POSITION_X,
>                                              0, 12440, 4, 0);
>                         input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
>                                              0, 8640, 4, 0);
> -               }
> -               else if (wacom_wac->shared->touch->product == 0x360) {
> +               } else if (hdev->product == 0x360) {
>                         input_set_abs_params(input_dev, ABS_MT_POSITION_X,
>                                              0, 8960, 4, 0);
>                         input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
>                                              0, 5920, 4, 0);
> -               }
> -               else if (wacom_wac->shared->touch->product == 0x393) {
> +               } else if (hdev->product == 0x393) {
>                         input_set_abs_params(input_dev, ABS_MT_POSITION_X,
>                                              0, 6400, 4, 0);
>                         input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> @@ -4039,8 +4039,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>                 fallthrough;
>
>         case WACOM_27QHDT:
> -               if (wacom_wac->shared->touch->product == 0x32C ||
> -                   wacom_wac->shared->touch->product == 0xF6) {
> +               if (hdev->product == 0x32C ||
> +                   hdev->product == 0xF6) {
>                         input_dev->evbit[0] |= BIT_MASK(EV_SW);
>                         __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
>                         wacom_wac->has_mute_touch_switch = true;
> --
> 2.55.0.887.g758fc8c411-goog
>
>