Re: [PATCH] HID: lg-g15: cancel pending work on remove to fix a use-after-free

[Hans de Goede]

Hi,

On 18-Jun-26 09:06, Maoyi Xie wrote:
> lg_g15_data is allocated with devm and holds a work item. The report
> handlers schedule that work straight from device input.
> lg_g15_event() and lg_g15_v2_event() do it on the backlight cycle key,
> and lg_g510_leds_event() does it too. The worker dereferences the
> lg_g15_data back through container_of.
> 
> The driver had no remove callback and never cancelled the work. So if a
> report scheduled the work and the keyboard was then unplugged, devres
> freed lg_g15_data while the work was still pending or running, and the
> worker touched freed memory. This is a use-after-free. It is reachable
> as a race on device unplug.
> 
> Add a remove callback that cancels the work before devres frees the
> state. g15->work is only initialized for the models that schedule it
> (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so guard the
> cancel on g15->work.func to avoid cancelling a work that was never set
> up. The g15 NULL test mirrors the one already in lg_g15_raw_event().
> 
> Fixes: 97b741aba918 ("HID: lg-g15: Add keyboard and LCD backlight control")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Hans de Goede <hansg@xxxxxxxxxx>
> Signed-off-by: Maoyi Xie <maoyixie.tju@xxxxxxxxx>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Regards,

Hans



> ---
>  drivers/hid/hid-lg-g15.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 1a88bc44ada4..02ef3e2094b4 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
> @@ -1374,11 +1374,27 @@ static const struct hid_device_id lg_g15_devices[] = {
>  };
>  MODULE_DEVICE_TABLE(hid, lg_g15_devices);
>  
> +static void lg_g15_remove(struct hid_device *hdev)
> +{
> +	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +
> +	/*
> +	 * g15->work is only initialized for the models that schedule it
> +	 * (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so only
> +	 * cancel it when it was set up.
> +	 */
> +	if (g15 && g15->work.func)
> +		cancel_work_sync(&g15->work);
> +
> +	hid_hw_stop(hdev);
> +}
> +
>  static struct hid_driver lg_g15_driver = {
>  	.name			= "lg-g15",
>  	.id_table		= lg_g15_devices,
>  	.raw_event		= lg_g15_raw_event,
>  	.probe			= lg_g15_probe,
> +	.remove			= lg_g15_remove,
>  };
>  module_hid_driver(lg_g15_driver);
>