Re: [PATCH v2] HID: hid-lenovo-go: cancel cfg_setup work in hid_go_cfg_remove()

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On July 7, 2026 2:06:12 AM PDT, "Günther Noack" <gnoack@google.com> wrote:
>Hello Derek and Mark!
>
>May I please draw your attention to this issue in the hid-lenovo drivers,
>which was pointed out by Sashiko:
>
>
>On Fri, May 15, 2026 at 06:23:46PM +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
>> - [Critical] Hardware state corruption and crashes due to use of a static global struct for device data.
>^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>
>> > diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
>> > index d4d26c7833563..b9d8cde531365 100644
>> > --- a/drivers/hid/hid-lenovo-go.c
>> > +++ b/drivers/hid/hid-lenovo-go.c
>> > @@ -2408,6 +2408,15 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>> >  
>> >  static void hid_go_cfg_remove(struct hid_device *hdev)
>> >  {
>> > +	/*
>> > +	 * cfg_setup is scheduled from hid_go_cfg_probe() with a 2 ms delay
>> > +	 * and dereferences drvdata.hdev.  Drain it here before tearing
>> > +	 * down so the workqueue cannot run after hid_destroy_device()'s
>> > +	 * put_device() has released the underlying hdev and dereference
>> > +	 * a stale drvdata.hdev pointer.
>> > +	 */
>> > +	cancel_delayed_work_sync(&drvdata.go_cfg_setup);
>> 
>> This isn't a bug introduced by this patch, but does the driver use a global
>> static variable (static struct hid_go_cfg drvdata) to store device state?
>> 
>> A second device probe would unconditionally overwrite drvdata.hdev and
>> re-initialize both drvdata.cfg_mutex and drvdata.go_cfg_setup.
>> 
>> Re-initializing an active mutex destroys its wait lists, and re-initializing
>> an active delayed work overwrites its linked list pointers, leading to
>> immediate memory corruption and kernel panics.
>> 
>> Should the device state be dynamically allocated per-instance instead?
>
>
>This is the same issue which was also introduced in hid-oxp, which I
>pointed out in [1].  As the review-bot points out above, while the data in
>your static drvdata variable is maybe only necessary once (but that
>also seems questionable?), I think the mutex and workqueue shouldn't
>just get re-initialized in the middle of being used, when a second device is
>plugged in and the probe() hook runs again.
>

Hi Günther,

While this is technically correct, the likelihood of this happening is realistically zero. All of these drivers are used for handheld devices with built-in HID controllers. There is nothing to "plug-in" per say.

That being said, I do intend to return to these drivers and fix that issue. I was waiting to do that after the claw driver is merged as it is essentially dealing with the same issues and that will provide a blueprint for all the needed changes in the others. 

Thanks,
Derek

>This pattern is used in both the drivers hid-lenovo-go.c, hid-lenovo-go-s.c,
>as well as in the newer hid-oxp discussed in [1].
>
>Shouldn't the drvdata just be allocated with one of the devm_*alloc() variants
>as in the other drivers?
>
>—Günther
>
>
>[1] https://lore.kernel.org/all/akfGTjId84EjV141@google.com/