Re: hid-lg-g15: possible use-after-free of devm data via work scheduled from a report

[Hans de Goede]

Hi,

On 17-Jun-26 14:10, Maoyi Xie wrote:
> Hi all,
> 
> I think the lg-g15 driver can read freed memory on disconnect. I would
> appreciate it if you could check my reading before I send a patch.
> 
> The per device state is allocated with devm in lg_g15_probe().
> 
> 	g15 = devm_kzalloc(&hdev->dev, sizeof(*g15), GFP_KERNEL);
> 
> That struct contains a work item, and the report handlers schedule it
> straight from device input. For example in lg_g15_event().
> 
> 	/* Backlight cycle button pressed? */
> 	if (data[1] & 0x80)
> 		schedule_work(&g15->work);
> 
> The same schedule_work(&g15->work) call also runs in lg_g15_v2_event() and
> lg_g510_leds_event(). The worker lg_g15_leds_changed_work() does a
> container_of() back to g15 and dereferences g15->mutex and g15->leds.
> 
> The driver has a probe but no remove callback, and there is no
> cancel_work_sync() anywhere in the file. So if a report schedules the work
> and the keyboard is then removed, devm frees g15 while the work is still
> pending or running, and the worker touches the freed object.
> 
> The attacker model is a Logitech G15 class keyboard that sends one report
> with the backlight cycle bit set and then disconnects. That can be a
> malicious device or an unlucky unplug.
> 
> I reproduced the freed while pending pattern under KASAN on 7.1-rc7. The
> workqueue picked up the orphaned work after the object was freed, and KASAN
> reported a slab-use-after-free read.
> 
> The fix I tried is a small remove callback that cancels the work before the
> devm teardown frees g15.
> 
> 	static void lg_g15_remove(struct hid_device *hdev)
> 	{
> 		struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> 
> 		if (g15)
> 			cancel_work_sync(&g15->work);
> 
> 		hid_hw_stop(hdev);
> 	}
> 
> and wiring it up with .remove = lg_g15_remove. The g15 NULL guard mirrors the
> existing check in lg_g15_raw_event().
> 
> Does this look like a real issue to you, and is the remove plus
> cancel_work_sync the approach you would want? If so I am happy to send a
> proper patch with a Fixes tag against 97b741aba918.

Thank you for reporting this. Yes this looks like a real issue (possible race
on device unplug).

As for your suggested solution, that looks good but g15->work is not always
initialized. For example in the g15->model == G13 case lg_g15_probe() does
not initialize it.

I don't think you should cancel an uninitialized work. Trying to queue it
will cause a WARN() backtrace to trigger, not sure if cancel also enforces
this.

So you should add a test for `g15->work.func != NULL` or just `g15->work.func`
before cancelling.

With that fixed, a proper patch fixing this would be much appreciated.

Regards,

Hans