[PATCH 0/1] HID: wacom: fix sibling use-after-free in mode change

[PATCH 0/1] HID: wacom: fix sibling use-after-free in mode change

[Jinmo Yang]

Hi,

I found the following use-after-free in the wacom HID driver while fuzzing
with syzkaller, and reproduced it deterministically on hid.git master
(fe2ec83746e5, v7.3-rc4 based) with a uhid reproducer -- 10 KASAN reports
out of 10 boots:

  BUG: KASAN: slab-use-after-free in wacom_parse_and_register+0x4fa8/0x58e0
  Read of size 8 at addr ffff88800be431f8 by task kworker/0:2/75

  CPU: 0 UID: 0 PID: 75 Comm: kworker/0:2 Not tainted 7.3.0-rc4-gfe2ec83746e5-dirty #2 PREEMPT(lazy)
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-debian-1.17.0-1 04/01/2014
  Workqueue: events wacom_mode_change_work
  Call Trace:
   <TASK>
   dump_stack_lvl+0xba/0x110
   print_report+0x153/0x4c6
   ? wacom_parse_and_register+0x4fa8/0x58e0
   ? __virt_addr_valid+0x221/0x4e0
   ? wacom_parse_and_register+0x4fa8/0x58e0
   kasan_report+0xe4/0x1a0
   ? wacom_parse_and_register+0x4fa8/0x58e0
   wacom_parse_and_register+0x4fa8/0x58e0
   ? do_raw_spin_lock+0x123/0x260
   ? __pfx_wacom_parse_and_register+0x10/0x10
   ? lockdep_hardirqs_on_prepare+0xdc/0x190
   ? _raw_spin_unlock_irqrestore+0x40/0x50
   ? trace_hardirqs_on+0x19/0x190
   ? _raw_spin_unlock_irqrestore+0x40/0x50
   wacom_mode_change_work+0x333/0x7e0
   process_one_work+0xa3b/0x19b0
   ? __pfx_process_one_work+0x10/0x10
   ? lock_acquire+0x18c/0x300
   ? lock_is_held_type+0x87/0xf0
   ? __pfx_wacom_mode_change_work+0x10/0x10
   worker_thread+0x5eb/0xe50
   ? __pfx_worker_thread+0x10/0x10
   ? kthread+0x13a/0x450
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x368/0x450
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x617/0x9e0
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x7ec/0x10f0
   ? pirq_enable_irq.cold+0xb7/0x199
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>

  Allocated by task 11:
   kasan_save_stack+0x30/0x50
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x7f/0x90
   __kmalloc_node_track_caller_noprof+0x26b/0x6e0
   devm_kmalloc+0xa2/0x290
   wacom_probe+0xb2/0xdb0
   hid_device_probe+0x4fb/0x850
   really_probe+0x235/0x760
   __driver_probe_device+0x291/0x3c0
   driver_probe_device+0x4a/0x140
   __device_attach_driver+0x1d2/0x270
   bus_for_each_drv+0x152/0x1d0
   __device_attach+0x1e1/0x470
   device_initial_probe+0xaf/0xd0
   bus_probe_device+0x62/0x160
   device_add+0x111a/0x1930
   hid_add_device+0x2c2/0x460
   uhid_device_add_worker+0x37/0x70
   process_one_work+0xa3b/0x19b0
   worker_thread+0x5eb/0xe50
   kthread+0x368/0x450
   ret_from_fork+0x617/0x9e0
   ret_from_fork_asm+0x1a/0x30

  Freed by task 82:
   kasan_save_stack+0x30/0x50
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x70
   __kasan_slab_free+0x47/0x70
   kfree+0x1b3/0x550
   release_nodes+0xcc/0x130
   devres_release_group+0x1c2/0x2d0
   hid_device_remove+0x107/0x200
   device_remove+0xc8/0x180
   device_release_driver_internal+0x448/0x610
   bus_remove_device+0x2b6/0x460
   device_del+0x378/0xd10
   hid_destroy_device+0x1a6/0x250
   uhid_char_release+0xeb/0x1f0
   __fput+0x3fd/0xb50
   fput_close_sync+0x113/0x240
   __x64_sys_close+0x8b/0x120
   do_syscall_64+0x106/0x5f0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

  The buggy address belongs to the object at ffff88800be43000
   which belongs to the cache kmalloc-2k of size 2048
  The buggy address is located 504 bytes inside of
   freed 2048-byte region [ffff88800be43000, ffff88800be43800)

  The buggy address belongs to the physical page:
  page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0xbe40
  head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
  flags: 0x100000000000040(head|node=0|zone=1)
  page_type: f5(slab)
  raw: 0100000000000040 ffff888008c42000 dead000000000100 dead000000000122
  raw: 0000000000000000 0000000000080008 00000000f5000000 0000000000000000
  head: 0100000000000040 ffff888008c42000 dead000000000100 dead000000000122
  head: 0000000000000000 0000000000080008 00000000f5000000 0000000000000000
  head: 0100000000000003 fffffffffffffe01 00000000ffffffff 00000000ffffffff
  head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
  page dumped because: kasan: bad access detected

  Memory state around the buggy address:
   ffff88800be43080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
   ffff88800be43100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  >ffff88800be43180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                                                  ^
   ffff88800be43200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
   ffff88800be43280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ==================================================================

wacom_mode_change_work() obtains both siblings' struct wacom from
wacom_shared and tears down and rebuilds them, holding no reference to
either. wacom_remove() cancels only the work owned by the device being
removed, so a worker owned by the other sibling keeps using this device
after its remove callback returns; hid_device_remove() then releases the
driver's devres group and frees struct wacom. Closing one sibling's
/dev/uhid descriptor is all it takes, with no privilege.

The report above is the read. On the syzkaller instance where I first hit
this -- a KASAN linux-next build, not the hid.git master tree I used for
the 10-boot runs -- the same worker also produced slab-use-after-free
*writes*, at features->touch_max in wacom_feature_mapping(), four bytes
at a fixed offset into the freed object with the value taken from a
feature report; and NULL dereferences in wacom_set_shared_values(), where
the sibling's wacom_release_resources() had cleared wacom_wac->shared
while this worker slept in wacom_parse_and_register().

The defect dates to 4082da80f46a ("HID: wacom: generic: add mode change
touch key") in v4.12, which introduced the worker, the reference-free
sibling lookup and the incomplete cancellation in one go. The lifetime it
depends on was already in place in that commit's parent.

This is also the issue 3a523ecf9ac3 ("HID: wacom: Fix Use-After-Free in
wacom_bamboo_pad") refers to when it notes that "lockless access in the
latter [wacom_mode_change_work] remains a pre-existing issue". That
series fixed the lifetime of the shared pointers; this patch fixes the
lifetime of the struct wacom objects the worker caches across
hid_hw_stop() and wacom_parse_and_register().

Fix and testing
===============
The patch serializes mode-change workers with wacom_remove() using a
driver-wide mutex. Removal cancels its own work before taking the mutex,
so a worker blocked on it cannot deadlock removal. Because
wacom_add_shared_data() registers its devres action inside the group
wacom_parse_and_register() opens, the wacom_release_resources() call in
wacom_remove() clears this device's shared pen or touch pointer while the
mutex is held, and a worker that starts later cannot select it.

x86_64, CONFIG_KASAN_GENERIC=y, CONFIG_PROVE_LOCKING=y,
CONFIG_DEBUG_ATOMIC_SLEEP=y, CONFIG_DETECT_HUNG_TASK=y, one fresh
headless QEMU guest per round:

                                  unpatched   patched
    rounds                               10        10
    KASAN use-after-free                 10         0
    lockdep reports                       0         0
    hung task / RCU stall                 0         0
    WARNING / BUG / oops                  0         0
    completed the full workload           0        10
    wacom interfaces bound               40        60
    input devices registered             60       150

The unpatched kernel faults about 4.6 s into every round, which is why it
binds and registers less. On the patched kernel the 150 input registrations
against 60 bound interfaces are re-registrations, and in this workload only
wacom_mode_change_work() re-registers, so the serialized path ran about
ninety times under lockdep without a report. Three further boots of the
functional path, two siblings bound and unbound with no mode-change report,
registered both devices every time with no KASAN report, warning or oops.

To make the existing window deterministic I added a one-second delay to
wacom_set_shared_values() under has_mode_change. That is test-only
instrumentation and is not part of the patch; with it the patched kernel
holds the new mutex across the full teardown and rebuild, which is the
state a lock-order problem would show up in.

Cost of this lock, and what is not fixed
========================================
The worker holds the new driver-wide mutex across hid_hw_stop() and
wacom_parse_and_register(), and that path issues synchronous feature
reports: wacom_retrieve_hid_descriptor() -> wacom_parse_hid() ->
wacom_feature_mapping() calls wacom_get_report() for HID_DG_CONTACTMAX
and, unguarded, for each of WACOM_HID_WD_OFFSET{LEFT,TOP,RIGHT,BOTTOM}.
On a uhid device every one of those waits up to 5 seconds in
__uhid_report_queue_and_wait() if the client does not answer, so a uhid
client that declares those usages and stays silent can hold the mutex for
tens of seconds and block the unbind of unrelated wacom devices. Without
the patch that path takes no lock, so this cross-device blocking is new.

I kept the global mutex because this is the minimal shape I could verify
for a fix going to stable, and because a lock inside wacom_shared cannot
work here: the worker's own devres release can drop the last reference to
that object, which is one of the windows being closed. A per sibling
group lock with a lifetime independent of the shared data is the obvious
narrower fix, and I am happy to follow up with it if you would rather
have that than the global mutex.

wacom_wireless_work() has the same shape as the worker fixed here: it
takes sibling struct wacom pointers from usb_get_intfdata() with no
reference and calls wacom_release_resources() and
wacom_parse_and_register() on them. It is USB-only and my reproducer does
not reach it, so I left it alone rather than extend a fix I cannot test.

The patch applies to hid.git master. It does not apply to for-next:
480c9cb14ae8 ("HID: wacom: Redesign shared sibling data lifecycle")
rewrote the same hunks and made shared->pen and shared->touch __rcu. The
same design works there with rcu_access_pointer() for the snapshot, and I
can send that variant if you prefer it.

Happy to send the reproducer and the serial logs off-list.

Thanks,
Jinmo

Jinmo Yang (1):
  HID: wacom: serialize mode changes with device removal

 drivers/hid/wacom_sys.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)


base-commit: fe2ec83746e501645709761605c2464a44fd2929
--
2.53.0

[PATCH 1/1] HID: wacom: serialize mode changes with device removal

[Jinmo Yang]

wacom_mode_change_work() takes both siblings' struct wacom out of
wacom_shared and tears down and rebuilds them, holding no reference to
either. wacom_remove() cancels only the work owned by the device being
removed, so a worker owned by the other sibling keeps using this device
after its remove callback returns, and hid_device_remove() then frees
struct wacom along with the driver's devres group.

Reproduced under KASAN on hid.git master (fe2ec83746e5) with a uhid
reproducer that binds a sibling pair, sends a mode-change report and then
closes one sibling's descriptor, 10 boots out of 10:

  BUG: KASAN: slab-use-after-free in wacom_parse_and_register+0x4fa8/0x58e0
  Read of size 8 at addr ffff88800be431f8 by task kworker/0:2/75
  Workqueue: events wacom_mode_change_work
   wacom_parse_and_register+0x4fa8/0x58e0
   wacom_mode_change_work+0x333/0x7e0
   process_one_work+0xa3b/0x19b0
  Allocated by task 11:  devm_kmalloc <- wacom_probe
  Freed by task 82:      devres_release_group <- hid_device_remove,
                         under uhid_char_release <- __x64_sys_close

The same object is also written: wacom_feature_mapping() stores
features->touch_max into it, four bytes at a fixed offset, with the value
taken from a feature report. Closing one sibling's /dev/uhid descriptor is
all it takes, with no privilege, and a reference on the hid_device would
not help: struct wacom is devm_kzalloc() on &hdev->dev.

Serialize mode-change workers with wacom_remove() using a driver-wide
mutex, cancelling the device's own work first so that a worker blocked on
the mutex cannot deadlock removal. wacom_add_shared_data() registers its
devres action inside the group wacom_parse_and_register() opens, so
wacom_remove() clears this device's shared pen or touch pointer under the
mutex and no later worker can pick it up.

The worker also snapshots both shared pointers and returns on a NULL
wacom_wac.shared, because releasing the first sibling's devres can drop
the last reference to the shared data.

Fixes: 4082da80f46a ("HID: wacom: generic: add mode change touch key")
Cc: stable@vger.kernel.org
Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
---
Applies to hid.git master (fe2ec83746e5). It does not apply to for-next;
see the cover letter.

 drivers/hid/wacom_sys.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 40770affdbde..ecd7b38b1418 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -764,6 +764,7 @@ struct wacom_hdev_data {
 
 static LIST_HEAD(wacom_udev_list);
 static DEFINE_MUTEX(wacom_udev_list_lock);
+static DEFINE_MUTEX(wacom_mode_change_lock);
 
 static bool wacom_are_sibling(struct hid_device *hdev,
 		struct hid_device *sibling)
@@ -2783,22 +2784,32 @@ static void wacom_remote_work(struct work_struct *work)
 static void wacom_mode_change_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, mode_change_work);
-	struct wacom_shared *shared = wacom->wacom_wac.shared;
+	struct wacom_shared *shared;
+	struct hid_device *pen;
+	struct hid_device *touch;
 	struct wacom *wacom1 = NULL;
 	struct wacom *wacom2 = NULL;
-	bool is_direct = wacom->wacom_wac.is_direct_mode;
+	bool is_direct;
 	int error = 0;
 
-	if (shared->pen) {
-		wacom1 = hid_get_drvdata(shared->pen);
+	mutex_lock(&wacom_mode_change_lock);
+	shared = wacom->wacom_wac.shared;
+	if (!shared)
+		goto out;
+	pen = shared->pen;
+	touch = shared->touch;
+	is_direct = wacom->wacom_wac.is_direct_mode;
+
+	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;
@@ -2808,16 +2819,17 @@ static void wacom_mode_change_work(struct work_struct *work)
 	if (wacom1) {
 		error = wacom_parse_and_register(wacom1, false);
 		if (error)
-			return;
+			goto out;
 	}
 
 	if (wacom2) {
 		error = wacom_parse_and_register(wacom2, false);
 		if (error)
-			return;
+			goto out;
 	}
 
-	return;
+out:
+	mutex_unlock(&wacom_mode_change_lock);
 }
 
 static int wacom_probe(struct hid_device *hdev,
@@ -2916,6 +2928,10 @@ static void wacom_remove(struct hid_device *hdev)
 	cancel_work_sync(&wacom->battery_work);
 	cancel_work_sync(&wacom->remote_work);
 	cancel_work_sync(&wacom->mode_change_work);
+
+	/* A sibling's mode-change work can also access this device. */
+	mutex_lock(&wacom_mode_change_lock);
+
 	timer_delete_sync(&wacom->idleprox_timer);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
@@ -2925,6 +2941,8 @@ static void wacom_remove(struct hid_device *hdev)
 
 	if (wacom->wacom_wac.features.type != REMOTE)
 		wacom_release_resources(wacom);
+
+	mutex_unlock(&wacom_mode_change_lock);
 }
 
 static int wacom_resume(struct hid_device *hdev)
-- 
2.53.0

[PATCH v2 0/1] HID: wacom: fix sibling use-after-free in mode change

[Jinmo Yang]

Hi,

Please drop v1 and take this instead. v1 carries a Fixes tag and Cc: stable
but does not fix what it claims to, and it adds a second problem.

The Sashiko AI review of v1 is right: wacom_remove() stops the hardware and
cancels the delayed work before it takes the new mutex, so a sibling worker
holding that mutex can bring the device back up afterwards through
wacom_parse_and_register(), and removal never repeats those steps. Two
things escape:

 - hidraw stays registered on an unbound device. hidraw_disconnect() is
   reached only from hid_disconnect(), which is reached only from
   hid_hw_stop(); neither hid_destroy_device() nor hid_remove_device()
   touches it. That one is from reading the code.

 - init_work is re-armed on memory devres is about to free, because
   wacom_parse_and_register() calls wacom_query_tablet_data() ->
   schedule_delayed_work(&wacom->init_work, 1s). That one KASAN catches.

Moving my test-only delay to just after the worker's hid_hw_stop(), so that
a concurrent wacom_remove() runs its own hid_hw_stop() inside it, gives this
on v1 five boots out of five:

  BUG: KASAN: slab-use-after-free in __run_timer_base.part.0
  Write of size 8 at addr ffff88800bc1b460 by task swapper/0/0
   run_timer_softirq / handle_softirqs / sysvec_apic_timer_interrupt
  Allocated by task 11:  devm_kmalloc <- wacom_probe
  Freed by task 79:      devres_release_group <- hid_device_remove,
                         under uhid_char_release <- __x64_sys_close

1120 bytes into the freed object is inside wacom->init_work.timer
(offsetof(struct wacom, init_work) is 984 and its timer sits at +72 here,
from vmlinux DWARF).

My v1 testing missed all of this because the delay sat in
wacom_set_shared_values(), after the worker's hid_hw_start(), so the window
never opened. The clean v1 result was real but it was not testing this.

Changes in v2, all in wacom_remove(); the worker is unchanged:

 - Take the mutex before hid_hw_close()/hid_hw_stop() rather than after,
   and move the remaining cancel_*_work_sync() calls and
   timer_delete_sync(&wacom->idleprox_timer) inside it, so the whole
   teardown is covered.
 - Keep cancel_work_sync(&wacom->mode_change_work) before the mutex, where
   it has to be: a worker blocked on the mutex would deadlock it.
 - Add a second cancel_work_sync(&wacom->mode_change_work) after the
   unlock, because moving hid_hw_stop() inside the mutex lets a report
   queue our own work again after the first cancel. That work can only find
   a NULL wacom_wac.shared and return, and the mutex is free by then so it
   cannot deadlock.

Same kernel and reproducer as v1 (x86_64, hid.git master, KASAN,
PROVE_LOCKING, one fresh QEMU guest per round). v2 is clean 10/10 with the
original delay placement and 5/5 with the one above, no lockdep report and
no leftover /sys/class/hidraw node; unpatched is 10/10 KASAN. The cost of
the global mutex and the untouched wacom_wireless_work() are as described
in v1 -- v2 only widens the hold to cover removal's own hid_hw_stop().

v1: https://lore.kernel.org/linux-input/20261004111353.118025-1-jinmo44.yang@gmail.com/
Sashiko: https://lore.kernel.org/linux-input/20261004112812.460B21F000FF@smtp.kernel.org/

Thanks,
Jinmo

Jinmo Yang (1):
  HID: wacom: serialize mode changes with device removal

 drivers/hid/wacom_sys.c | 60 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 10 deletions(-)


base-commit: fe2ec83746e501645709761605c2464a44fd2929
-- 
2.53.0

[PATCH v2] HID: wacom: serialize mode changes with device removal

[Jinmo Yang]

wacom_mode_change_work() takes both siblings' struct wacom out of
wacom_shared and tears down and rebuilds them, holding no reference to
either. wacom_remove() cancels only the work owned by the device being
removed, so a worker owned by the other sibling keeps using this device
after its remove callback returns, and hid_device_remove() then frees
struct wacom along with the driver's devres group.

Reproduced under KASAN on hid.git master, 10 boots out of 10:

  BUG: KASAN: slab-use-after-free in wacom_parse_and_register+0x4fa8/0x58e0
  Read of size 8 at addr ffff88800be431f8 by task kworker/0:2/75
  Workqueue: events wacom_mode_change_work
   wacom_parse_and_register+0x4fa8/0x58e0
   wacom_mode_change_work+0x333/0x7e0
   process_one_work+0xa3b/0x19b0
  Allocated by task 11:  devm_kmalloc <- wacom_probe
  Freed by task 82:      devres_release_group <- hid_device_remove,
                         under uhid_char_release <- __x64_sys_close

The same object is also written: wacom_feature_mapping() stores
features->touch_max into it, four bytes at a fixed offset, with the value
taken from a feature report. Closing one sibling's /dev/uhid descriptor is
all it takes, with no privilege, and a reference on the hid_device would
not help: struct wacom is devm_kzalloc() on &hdev->dev.

Serialize mode-change workers with wacom_remove() using a driver-wide
mutex, held across the whole teardown. Any step left outside it is undone
by a sibling worker and never repeated, because such a worker brings the
device back up through wacom_parse_and_register(): stopping the hardware
outside the mutex leaves hidraw registered on an unbound device, since
hidraw_disconnect() is reached only from hid_hw_stop(), and cancelling the
delayed work outside it leaves init_work armed on memory devres is about
to free.

Cancel this device's own mode-change work before taking the mutex, since a
worker blocked on it would deadlock that cancel, and once more after the
unlock, because a report processed before hid_hw_stop() can queue it again.
That worker can only find a NULL wacom_wac.shared and return.

Because wacom_add_shared_data() registers its devres action inside the
group wacom_parse_and_register() opens, the wacom_release_resources() call
under the mutex clears this device from the shared pen and touch pointers,
so a worker starting after the unlock cannot select it.

The worker also snapshots both shared pointers and returns on a NULL
wacom_wac.shared, because releasing the first sibling's devres can drop
the last reference to the shared data.

Fixes: 4082da80f46a ("HID: wacom: generic: add mode change touch key")
Cc: stable@vger.kernel.org
Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
---
 drivers/hid/wacom_sys.c | 60 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 40770affdbde..092e91ed1c3d 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -764,6 +764,7 @@ struct wacom_hdev_data {
 
 static LIST_HEAD(wacom_udev_list);
 static DEFINE_MUTEX(wacom_udev_list_lock);
+static DEFINE_MUTEX(wacom_mode_change_lock);
 
 static bool wacom_are_sibling(struct hid_device *hdev,
 		struct hid_device *sibling)
@@ -2783,22 +2784,32 @@ static void wacom_remote_work(struct work_struct *work)
 static void wacom_mode_change_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, mode_change_work);
-	struct wacom_shared *shared = wacom->wacom_wac.shared;
+	struct wacom_shared *shared;
+	struct hid_device *pen;
+	struct hid_device *touch;
 	struct wacom *wacom1 = NULL;
 	struct wacom *wacom2 = NULL;
-	bool is_direct = wacom->wacom_wac.is_direct_mode;
+	bool is_direct;
 	int error = 0;
 
-	if (shared->pen) {
-		wacom1 = hid_get_drvdata(shared->pen);
+	mutex_lock(&wacom_mode_change_lock);
+	shared = wacom->wacom_wac.shared;
+	if (!shared)
+		goto out;
+	pen = shared->pen;
+	touch = shared->touch;
+	is_direct = wacom->wacom_wac.is_direct_mode;
+
+	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;
@@ -2808,16 +2819,17 @@ static void wacom_mode_change_work(struct work_struct *work)
 	if (wacom1) {
 		error = wacom_parse_and_register(wacom1, false);
 		if (error)
-			return;
+			goto out;
 	}
 
 	if (wacom2) {
 		error = wacom_parse_and_register(wacom2, false);
 		if (error)
-			return;
+			goto out;
 	}
 
-	return;
+out:
+	mutex_unlock(&wacom_mode_change_lock);
 }
 
 static int wacom_probe(struct hid_device *hdev,
@@ -2905,6 +2917,21 @@ static void wacom_remove(struct hid_device *hdev)
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 	struct wacom_features *features = &wacom_wac->features;
 
+	/*
+	 * Cancel our own mode-change work first: a worker blocked on the
+	 * mutex below would deadlock this cancel if the order were reversed.
+	 */
+	cancel_work_sync(&wacom->mode_change_work);
+
+	/*
+	 * A sibling's mode-change work tears this device down and brings it
+	 * back up, so hold the mutex across the whole teardown. Stopping the
+	 * hardware outside it would let such a worker run hid_hw_start() for
+	 * this device afterwards and leave hidraw registered on a device that
+	 * is about to be unbound.
+	 */
+	mutex_lock(&wacom_mode_change_lock);
+
 	if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
 		hid_hw_close(hdev);
 
@@ -2915,7 +2942,6 @@ static void wacom_remove(struct hid_device *hdev)
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
 	cancel_work_sync(&wacom->remote_work);
-	cancel_work_sync(&wacom->mode_change_work);
 	timer_delete_sync(&wacom->idleprox_timer);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
@@ -2923,8 +2949,22 @@ static void wacom_remove(struct hid_device *hdev)
 	/* make sure we don't trigger the LEDs */
 	wacom_led_groups_release(wacom);
 
+	/*
+	 * Releasing our resources runs the wacom_remove_shared_data() devres
+	 * action, which clears this device from the shared pen and touch
+	 * pointers, so no worker started after the unlock can select it.
+	 */
 	if (wacom->wacom_wac.features.type != REMOTE)
 		wacom_release_resources(wacom);
+
+	mutex_unlock(&wacom_mode_change_lock);
+
+	/*
+	 * A report processed before hid_hw_stop() may have queued our work
+	 * again. It can only find a NULL wacom_wac.shared and return now, but
+	 * it must not still be running when devres frees this device.
+	 */
+	cancel_work_sync(&wacom->mode_change_work);
 }
 
 static int wacom_resume(struct hid_device *hdev)

base-commit: fe2ec83746e501645709761605c2464a44fd2929
-- 
2.53.0