Re: [PATCH v3 8/8] HID: wacom: do not use 'driver_data'

On Mon, Jul 27, 2026 at 8:26=E2=80=AFAM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issu=
e(s) to consider:
>
> New issues:
> - [Critical] Removal of driver_data breaks dynamic device binding via sys=
fs 'new_id', preventing testing of unknown/clone devices.
>
> Pre-existing issues:
> - [High] Missing cancellation of `wacom->init_work` on the error path for=
 wireless monitors leads to a UAF.
> --
>
> --- Patch [8]: [PATCH v3 8/8] HID: wacom: do not use 'driver_data' ---
> commit 147dba29dc34be167b7150ce20799c7b7dd39442
> Author: Pawel Zalewski (The Capable Hub) <[email protected]>
>
> HID: wacom: do not use 'driver_data'
>
> This patch removes the use of driver_data for looking up product features=
 in
> the wacom driver. It instead looks up wacom_features directly by product =
ID
> to reduce macro complexity and simplify the code.
>
> [ ... ]
>
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 834519751a5cf..4514b2762cc33 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -2848,7 +2845,7 @@ static int wacom_probe(struct hid_device *hdev,
> >
> >       wacom_wac =3D &wacom->wacom_wac;
> >
> > -     const struct wacom_features *id_features =3D get_wacom_features(i=
d->driver_data);
> > +     const struct wacom_features *id_features =3D get_wacom_features(i=
d->product);
>
> [Severity: Critical]
> Does removing driver_data introduce a regression for dynamic device bindi=
ng
> via sysfs 'new_id'?

Both 6/8 and 8/8 are tested with legacy and new devices. Everything looked =
good.

> By strictly using get_wacom_features(id->product), the mechanism for inje=
cting
> unknown product IDs seems rendered unusable because the injected PID won'=
t match
> any entry in the static wacom_features_table. Previously, couldn't users =
pass a
> feature table index via driver_data to test unknown or clone devices?

This particular case doesn't cause trouble since the existing solution
already supports new devices that were not defined in the static
table. Hotplugging worked without problem.

> [Severity: High]
> This is a pre-existing issue, but is there a missing cancellation of
> wacom->init_work on the error path for wireless monitors?
>
> drivers/hid/wacom_sys.c:wacom_parse_and_register() {
>     ...
>         if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
>                 error =3D hid_hw_open(hdev);
>                 if (error) {
>                         hid_err(hdev, "hw open failed\n");
>                         goto fail_hw_stop;
>                 }
>         }
>     ...
> }
>
> In wacom_parse_and_register(), wacom_query_tablet_data() schedules
> wacom->init_work. If hid_hw_open() subsequently fails for a wireless moni=
tor
> (WACOM_DEVICETYPE_WL_MONITOR), the error path jumps to fail_hw_stop and
> returns without calling cancel_delayed_work_sync(&wacom->init_work).

From the logic above, I think we need to call
cancel_delayed_work_sync() inside the loop before goto fail_hw_stop.

Cheers,
Ping


> Because the wacom structure is managed by devres and will be freed upon p=
robe
> failure, could this result in a use-after-free when init_work executes la=
ter?
>
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260727-mod-devi=
[email protected]?part=3D8
>