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

[Jinmo Yang <jinmo44.yang@gmail.com>]

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