Thread (8 messages) 8 messages, 2 authors, 15h ago

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

From: sashiko-bot@kernel.org
Date: 2026-07-07 12:25:43

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

Pre-existing issues:
- [High] Concurrent sibling unbind causes Use-After-Free in wacom_mode_change_work()
--
--- Patch [4]: [PATCH 4/4] HID: wacom: Redesign shared sibling data lifecycle ---
commit 5aab8fc5d0e17a15712b387b6273c3b728adb79d
Author: Lee Jones [off-list ref]

HID: wacom: Redesign shared sibling data lifecycle

The Wacom driver coordinates state between sibling interfaces of the same
physical device using a shared structure 'wacom_shared' inside
'wacom_hdev_data'. This patch redesigns the sibling data lifecycle to use
stable static attributes for sibling matching and serializes refcount drops
in wacom_remove_shared_data().
quoted hunk ↗ jump to hunk
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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();
[Severity: High]
This is a pre-existing issue, but while reviewing these RCU lifecycle changes,
I noticed a potential use-after-free condition involving the pointers being
managed here.

When a sibling interface is concurrently unbound, wacom_remove_shared_data()
clears the pointer and calls synchronize_rcu() here.

However, looking at wacom_mode_change_work() in drivers/hid/wacom_sys.c, the
shared sibling pointers are accessed without holding an RCU read lock:

    struct hid_device *touch = rcu_access_pointer(shared->touch);

    if (touch) {
        wacom2 = hid_get_drvdata(touch);
        wacom_release_resources(wacom2);
        hid_hw_stop(wacom2->hdev);
    }

Because wacom_mode_change_work() does not enter an RCU read-side critical
section, synchronize_rcu() won't wait for it to finish before returning
and allowing the sibling device to be freed.

Could this sequence lead to a use-after-free if wacom_mode_change_work()
subsequently dereferences the touch device pointer after it has been freed
by the driver core?
-		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;
 	}
 }
-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707121051.1683937-1-lee@kernel.org?part=4
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help