[PATCH v3 1/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad

[PATCH v3 1/4] 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 rcu_read_lock() 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.

Finally, advertise the SW_MUTE_DEVICE capability on Touch input
devices prior to registration in wacom_setup_touch_input_capabilities()
to prevent invalid post-registration capability modifications.

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

 drivers/hid/wacom_sys.c | 41 ++++++++++++++----------
 drivers/hid/wacom_wac.c | 70 ++++++++++++++++++++++-------------------
 drivers/hid/wacom_wac.h |  2 +-
 3 files changed, 63 insertions(+), 50 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2220168bf116..7ba589826548 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -877,10 +877,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;
@@ -909,6 +915,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;
@@ -917,11 +928,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;
 }
 
@@ -2345,9 +2351,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);
+
+	mutex_lock(&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) {
@@ -2361,12 +2373,7 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
 			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);
-	}
+	mutex_unlock(&wacom_udev_list_lock);
 }
 
 static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index da1f0ea85625..495960227b8d 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -510,6 +510,22 @@ 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;
+
+	if (!wacom_wac->shared)
+		return;
+
+	rcu_read_lock();
+	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);
+	}
+	rcu_read_unlock();
+}
+
 static int wacom_intuos_pad(struct wacom_wac *wacom)
 {
 	struct wacom_features *features = &wacom->features;
@@ -650,12 +666,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);
@@ -2153,7 +2165,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)
@@ -2161,9 +2173,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;
 	}
@@ -3381,11 +3391,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]);
@@ -3420,11 +3427,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 */
@@ -3951,11 +3955,22 @@ 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 wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+	struct hid_device *hdev = wacom->hdev;
 	struct wacom_features *features = &wacom_wac->features;
 
 	if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
 		return -ENODEV;
 
+	if (features->type != TABLETPC &&
+	    features->type != TABLETPC2FG &&
+	    features->type != MTSCREEN &&
+	    features->type != MTTPC &&
+	    features->type != MTTPC_B) {
+		input_dev->evbit[0] |= BIT_MASK(EV_SW);
+		__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
+	}
+
 	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
 		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 	else
@@ -3992,22 +4007,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	switch (features->type) {
 	case INTUOSP2_BT:
 	case INTUOSP2S_BT:
-		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,
@@ -4037,10 +4047,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) {
-			input_dev->evbit[0] |= BIT_MASK(EV_SW);
-			__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
+		if (hdev->product == 0x32C ||
+		    hdev->product == 0xF6) {
 			wacom_wac->has_mute_touch_switch = true;
 			wacom_wac->is_soft_touch_switch = true;
 		}
@@ -4059,8 +4067,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 
 	case INTUOSHT:
 	case INTUOSHT2:
-		input_dev->evbit[0] |= BIT_MASK(EV_SW);
-		__set_bit(SW_MUTE_DEVICE, input_dev->swbit);
 		fallthrough;
 
 	case BAMBOO_PT:
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.54.0.1099.g489fc7bff1-goog

[PATCH v3 2/4] 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

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

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 7ba589826548..9b352027aa98 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -878,11 +878,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);
 			}
 		}
 
@@ -916,9 +923,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);
 
@@ -2356,7 +2363,11 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
 	mutex_lock(&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);
 		}
@@ -2797,16 +2808,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 495960227b8d..32d6f1dfb001 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3296,6 +3296,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;
 
 	/*
@@ -3308,13 +3309,12 @@ 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);
+	rcu_read_lock();
+	pen = rcu_dereference(wacom->shared->pen);
+	if (pen)
+		hid_input_report(pen, HID_INPUT_REPORT, data,
+				 WACOM_PKGLEN_PENABLED, 1);
+	rcu_read_unlock();
 
 	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.54.0.1099.g489fc7bff1-goog

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

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `wacom_mode_change_work()` misuses `rcu_access_pointer()` to fetch RCU-protected pointers without holding `rcu_read_lock()`, leading to a Use-After-Free.
--

commit 4eb3f98aa9f594548c94852bcf57eb40ae946cd8
Author: Lee Jones <lee@kernel.org>
Subject: HID: wacom: Fix Use-After-Free in wacom_bamboo_pad

This commit resolves a Use-After-Free vulnerability in
wacom_bamboo_pad_pen_event() by introducing RCU protection for the pen and
touch pointers within the wacom_shared structure. It annotates the pointers
with __rcu, wraps lockless read access with rcu_read_lock() and
rcu_dereference(), and updates writers using the appropriate RCU assignment
primitives. It also updates pointer access in wacom_mode_change_work() to use
rcu_access_pointer() to avoid warnings.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 7ba5898265481..9b352027aa980 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2797,16 +2808,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 isn't a bug introduced by this patch, but does wacom_mode_change_work()
misuse rcu_access_pointer() here by fetching RCU-protected pointers without
holding rcu_read_lock(), which still leaves an exploitable Use-After-Free?

If the sibling device is unbound concurrently, its wacom_remove() triggers
wacom_remove_shared_data(), which nullifies the shared pointer and calls
synchronize_rcu(). Because the worker does not hold rcu_read_lock(),
synchronize_rcu() will not wait for it.

Doesn't this allow the sibling device to be completely freed while the worker
is still actively dereferencing its memory via hid_get_drvdata(),
wacom_release_resources(), and hid_hw_stop()?

Could using rcu_access_pointer() (which is explicitly intended for pointers
that will not be dereferenced) be papering over the underlying race condition
just to silence sparse/lockdep warnings?

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

[PATCH v3 3/4] 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

 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 9b352027aa98..1b019e3331b4 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -759,27 +759,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;
 
 	/*
@@ -787,11 +807,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;
 	}
 
@@ -804,7 +824,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;
 
 	/*
@@ -812,17 +832,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;
 
 	/*
@@ -838,7 +858,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;
 		}
@@ -846,7 +866,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;
 		}
@@ -860,18 +880,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,
@@ -885,17 +902,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;
 	}
 }
@@ -918,7 +937,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.54.0.1099.g489fc7bff1-goog

[PATCH v3 4/4] HID: wacom: Fix teardown order in wacom_mode_change_work

[Lee Jones]

wacom_mode_change_work() called wacom_release_resources() before
hid_hw_stop(). wacom_release_resources() triggers devres cleanup
which calls wacom_remove_shared_data() to nullify wacom_wac->shared.
Since hid_hw_stop() was not called yet, hardware IRQs could still fire
and dereference the nullified wacom_wac->shared pointer, causing a crash.

Fix this by correcting the teardown order to call hid_hw_stop()
first, matching the order used in wacom_remove().

Fixes: 4082da80f46a ("HID: wacom: generic: add mode change touch key")
Signed-off-by: Lee Jones <lee@kernel.org>
---

v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes

 drivers/hid/wacom_sys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 1b019e3331b4..378dddbf7ec5 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2835,16 +2835,16 @@ static void wacom_mode_change_work(struct work_struct *work)
 
 	if (pen) {
 		wacom1 = hid_get_drvdata(pen);
-		wacom_release_resources(wacom1);
 		hid_hw_stop(wacom1->hdev);
+		wacom_release_resources(wacom1);
 		wacom1->wacom_wac.has_mode_change = true;
 		wacom1->wacom_wac.is_direct_mode = is_direct;
 	}
 
 	if (touch) {
 		wacom2 = hid_get_drvdata(touch);
-		wacom_release_resources(wacom2);
 		hid_hw_stop(wacom2->hdev);
+		wacom_release_resources(wacom2);
 		wacom2->wacom_wac.has_mode_change = true;
 		wacom2->wacom_wac.is_direct_mode = is_direct;
 	}
-- 
2.54.0.1099.g489fc7bff1-goog

Re: [PATCH v3 4/4] HID: wacom: Fix teardown order in wacom_mode_change_work

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing synchronization between sibling device removal and `wacom_mode_change_work` leads to Use-After-Free and double `hid_hw_stop()`.
- [Critical] Missing synchronization allows multiple `wacom_mode_change_work` instances to execute concurrently on both siblings, leading to double teardown and state corruption.
- [Critical] Error paths in `wacom_parse_and_register()` after `hid_hw_start()` fail to call `hid_hw_stop()`, leaving hardware IRQs active after releasing resources.
- [High] If `wacom_parse_and_register()` fails for the first sibling device, the second device is permanently left in a stopped and uninitialized state.
- [High] Pending background workqueues and timers are not cancelled before releasing device resources in `wacom_mode_change_work`, violating subsystem teardown guidelines and causing UAF.
- [High] `wacom_remove()` cancels `mode_change_work` after stopping the hardware, creating a race that leaves the device started and permanently leaks HID interfaces.
--

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

HID: wacom: Fix teardown order in wacom_mode_change_work

wacom_mode_change_work() called wacom_release_resources() before
hid_hw_stop(), which could lead to a crash if hardware IRQs fire and
dereference the nullified shared pointer. This commit fixes the teardown
order to call hid_hw_stop() first.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 1b019e3331b45..378dddbf7ec5b 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2835,16 +2835,16 @@ static void wacom_mode_change_work(struct work_struct *work)
>  
>  	if (pen) {

[Severity: Critical]
This is a pre-existing issue, but is it possible for the shared device
pointers to be dereferenced after they are freed?

In wacom_mode_change_work(), the sibling device pointers are fetched:

    struct hid_device *pen = rcu_access_pointer(shared->pen);
    ...

These pointers are accessed without holding an rcu_read_lock or acquiring
a device reference. If a sibling device is concurrently unbound,
wacom_remove() clears the shared pointer and calls synchronize_rcu(), but
it would not wait for this worker. Could this result in a use-after-free
when hid_hw_stop() is called on the stale pointer?

[Severity: Critical]
This isn't a bug introduced by this patch, but does this function have
sufficient synchronization if mode change events arrive concurrently on
both pen and touch interfaces?

Since the code unconditionally operates on both sibling devices without
locking:

    wacom1 = hid_get_drvdata(pen);
    hid_hw_stop(wacom1->hdev);
    wacom_release_resources(wacom1);

If both devices independently schedule their own mode_change_work, could
both workers run simultaneously and race to call wacom_release_resources()
on the exact same pointers, potentially causing a double-free in the
devres core?

>  		wacom1 = hid_get_drvdata(pen);
> -		wacom_release_resources(wacom1);
>  		hid_hw_stop(wacom1->hdev);
> +		wacom_release_resources(wacom1);

[Severity: High]
This is a pre-existing issue, but are we certain all background tasks
are cancelled before releasing resources here?

Unlike wacom_remove(), wacom_mode_change_work() does not cancel pending
tasks like init_work before calling wacom_release_resources(). Since
wacom_release_resources() destroys devres allocations (such as led groups),
could a delayed wacom_init_work() execute concurrently and access these
freed structures, resulting in a use-after-free?

>  		wacom1->wacom_wac.has_mode_change = true;
>  		wacom1->wacom_wac.is_direct_mode = is_direct;
>  	}
>  
>  	if (touch) {
>  		wacom2 = hid_get_drvdata(touch);
> -		wacom_release_resources(wacom2);
>  		hid_hw_stop(wacom2->hdev);
> +		wacom_release_resources(wacom2);
>  		wacom2->wacom_wac.has_mode_change = true;
>  		wacom2->wacom_wac.is_direct_mode = is_direct;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but what happens to the second
device if re-registration fails for the first one?

Looking at the rest of wacom_mode_change_work():

    if (wacom1) {
        error = wacom_parse_and_register(wacom1, false);
        if (error)
            return;
    }
    if (wacom2) {
        ...
    }

If wacom_parse_and_register() fails for wacom1, the early return bypasses
the initialization for wacom2. Does this permanently leave the touch
interface hardware stopped and its resources released without recovery?

[Severity: Critical]
This is a pre-existing issue, but is there a missing hardware teardown
in the error handling path of wacom_parse_and_register()?

When an error occurs after hid_hw_start() succeeds:

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

The goto fail jumps past hid_hw_stop(hdev). Does this leave the hid
hardware running while its resources are freed, allowing subsequent
hardware irqs to trigger wacom_raw_event() and cause a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does wacom_remove()
cancel the mode_change_work too late?

In wacom_remove(), the cancellation happens after stopping the hardware:

drivers/hid/wacom_sys.c:wacom_remove() {
    hid_hw_stop(hdev);
    cancel_delayed_work_sync(&wacom->init_work);
    ...
    cancel_work_sync(&wacom->mode_change_work);
}

If mode_change_work is pending and runs after hid_hw_stop(), it will
call wacom_parse_and_register(), which invokes hid_hw_start(hdev).
Once cancel_work_sync() returns, wacom_remove() completes without
re-stopping the hardware. Could this permanently leak the active hid
interfaces?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless TOCTOU Use-After-Free of sibling devices in wacom_mode_change_work
--

commit bb8e808fb26bf4a0ca1a10604a506fb1d60c8173
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 in
wacom_intuos_pad() that could lead to a use-after-free. It introduces
RCU protection for the touch_input pointer, wrapping lockless readers
with rcu_read_lock() and writers with rcu_assign_pointer().

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..7ba5898265481 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -877,10 +877,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 is a pre-existing issue, but is it possible for a sibling device to
still be used after this synchronize_rcu() call?

Looking at wacom_mode_change_work(), it reads shared->pen and shared->touch
locklessly:

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 context switch occurs right after shared->pen is evaluated as true,
could a sibling device be physically disconnected, triggering
wacom_remove_shared_data() to nullify shared->pen and free the object?

Because wacom_mode_change_work() does not hold an rcu_read_lock(), this
synchronize_rcu() wouldn't block it. When the workqueue resumes, could it
pass the now-freed pointer to hid_get_drvdata() and cause a use-after-free?

>  
>  		kref_put(&data->kref, wacom_release_shared_data);
>  		wacom_wac->shared = NULL;

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