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

[Jason Gerecke]

On Sun, Oct 4, 2026 at 8:59 PM Jinmo Yang <jinmo44.yang@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Jinmo Yang <jinmo44.yang@xxxxxxxxx>
> ---
>  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);

I'm not a huge fan of a driver-wide mutexes. An alternative would seem
to be having both siblings share a common `struct work_struct
mode_change_work` and then relying on similar on disable_work_sync
calls inside of wacom_remove to ensure the teardown/recreation fully
completes regardless of which device gets removed.

I asked an LLM for an example of this idea and it requires a bit more
work (probably too much to backport to stable) but I'd still be
interested in following up on that path... Maybe submit a two-part
patch--one that uses this mutex idea and which can be backported to
stable, and a second that then relaxes the driver-wide mutex into a
shared mode_change_work. Thoughts?

>
>  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);

Regardless of the comments above, I think it makes sense to swap this
out for a call to 'disable_work_sync' instead. The documentation for
that indicates that it does the same thing as 'cancel_work_sync' but
also prevents future attempts to schedule the work (which should let
you remove the second call to 'cancel_work_sync' at the end of this
function).

Jason (she/they)
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....


> +
> +       /*
> +        * 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
>
>