Re: [PATCH] HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing workqueue cancellation on module unload or device removal causes a Use-After-Free
- [High] Missing edge detection for Game Mode and brightness buttons leads to USB bus spam and state desynchronization
- [High] Data race on `g15->transfer_buf` during device probe
- [Medium] LED brightness callbacks violate the API contract by returning success on failure
- [Medium] Hardware events arriving before LED registration trigger a kernel WARN_ON
--

commit f98ec9abc4841148689435f75f93ed341ba49d7d
Author: Xavier Bestel <xav@bes.tel>

HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards

Add support for the Logitech G710 and G710+ gaming keyboards to the
hid-lg-g15 driver, exposing the G-keys, M-keys, game mode toggle, and
brightness cycle buttons.

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c

[ ... ]

> @@ -597,6 +605,234 @@ static int lg_g510_mkey_led_set(struct led_classdev *led_cdev,
>  	return ret;
>  }
>  
> +/******** G710 LED functions ********/

[ ... ]

> +static int lg_g710_mkey_led_set(struct led_classdev *led_cdev,
> +			  enum led_brightness brightness)
> +{

[ ... ]

> +	ret = hid_hw_raw_request(g15->hdev, LG_G710_FEATURE_M_KEYS_LEDS,
> +				 g15->transfer_buf, 2,
> +				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +	if (ret == 2) {
> +		/* Success */
> +		g15_led->brightness = brightness;
> +		ret = 0;
> +	} else {
> +		if (ret != -EPIPE)
> +			hid_err(g15->hdev, "Error setting LED brightness: %d\n", ret);
> +		/* -EPIPE is transient (USB stall), cache is unchanged, retry next time */
> +		ret = 0;
> +	}
> +
> +	mutex_unlock(&g15->mutex);
> +
> +	return ret;
> +}

Does returning 0 here break the LED class API contract when a hard error
like -EIO or -ENODEV occurs?

If we return success despite hardware failure, won't the internal software
cache update while the physical hardware remains unchanged, leading to a
permanent desynchronization between sysfs and the device?

[ ... ]

> @@ -890,6 +1136,71 @@ static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
>  	return 0;
>  }
>  
> +static int lg_g710_event(struct lg_g15_data *g15, u8 *data, int size)
> +{

[ ... ]

> +	/*
> +	 * Detect brightness key presses and schedule the work function
> +	 * to cycle cached brightness and notify userspace.
> +	 * Bit 10 = WASD backlight (maps to LG_G15_LCD_BRIGHTNESS slot).
> +	 * Bit 11 = Kbd backlight (maps to LG_G15_KBD_BRIGHTNESS slot).
> +	 */
> +	if (pressed_keys & BIT(10)) {
> +		set_bit(LG_G15_LCD_BRIGHTNESS, &g15->brightness_changed);
> +		schedule_work(&g15->work);
> +	}
> +	if (pressed_keys & BIT(11)) {
> +		set_bit(LG_G15_KBD_BRIGHTNESS, &g15->brightness_changed);
> +		schedule_work(&g15->work);
> +	}
> +
> +	/* Game mode toggle — bit 12 is a state bit, trigger on any change */
> +	if (pressed_keys & BIT(12)) {
> +		set_bit(LG_G15_GAMEMODE, &g15->brightness_changed);
> +		schedule_work(&g15->work);
> +	}

Could these bitwise checks cause continuous triggering on every HID report
received?

Since the game mode toggle check evaluates the absolute state, if game mode
is enabled (for instance, while holding a macro key), won't this continuously
schedule the workqueue, spam the USB bus with synchronous get-report requests,
and flood userspace with duplicate notifications?

Also, since the condition evaluates to false when game mode is disabled, will
userspace ever be notified of the disabled state?

Similarly for the brightness buttons, does checking the absolute state mean
that holding the button down will rapidly decrement the software brightness
cache and permanently desynchronize it from the physical hardware?

Should the driver track previous states and trigger actions only on edge
transitions instead?

[ ... ]

> @@ -1212,6 +1562,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	case LG_Z10:
>  		connect_mask = HID_CONNECT_HIDRAW;
>  		break;
> +	case LG_G710:
> +		INIT_WORK(&g15->work, lg_g710_leds_changed_work);
> +		hdev->quirks |= HID_QUIRK_NOGET;
> +		connect_mask = HID_CONNECT_DEFAULT;
> +		gkeys_settings_feature_report = LG_G710_FEATURE_EXTRA_KEYS;
> +		gkeys = 6;
> +		break;
>  	}
>  
>  	ret = hid_hw_start(hdev, connect_mask);

This isn't a bug introduced exclusively by this patch, but does the lack of a
remove callback or devres action to cancel the scheduled work risk a
use-after-free?

The driver allocates the context structure using devm_kzalloc and initializes
the work struct, but nothing calls cancel_work_sync on shutdown. If the work
is pending or running on the system workqueue when devm frees the memory,
won't it dereference freed memory?

[ ... ]

> @@ -1234,11 +1591,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	}
>  
>  	if (gkeys_settings_feature_report) {
> +		int report_size = (g15->model == LG_G710) ? gkeys * 2 : gkeys;
>  
>  		g15->transfer_buf[0] = gkeys_settings_feature_report;
> -		memset(g15->transfer_buf + 1, 0, gkeys);
> +		memset(g15->transfer_buf + 1, 0, report_size);
>  		ret = hid_hw_raw_request(g15->hdev,
>  				gkeys_settings_feature_report,
> -				g15->transfer_buf, gkeys + 1,
> +				g15->transfer_buf, report_size + 1,
>  				HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>  	}

This isn't a new bug, but since this lockless access to the transfer buffer
occurs right after hid_hw_start, could an incoming HID report concurrently
schedule the work queue?

The asynchronous worker safely locks the mutex before mutating the exact same
transfer buffer. Does this allow the workqueue to overwrite the buffer while
the probe function is actively using it, resulting in malformed requests?

[ ... ]

> @@ -1327,12 +1686,18 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto error_hw_stop;
>  
>  	/* Register LED devices */
> -	for (i = 0; i < LG_G15_LED_MAX; i++) {
> +	for (i = 0; i <= LG_G15_MACRO_RECORD; i++) {
>  		ret = lg_g15_register_led(g15, i, led_names[i]);
>  		if (ret)
>  			goto error_hw_stop;
>  	}
>  
> +	if (g15->model == LG_G710) {
> +		ret = lg_g15_register_led(g15, LG_G15_GAMEMODE, "g15::gamemode");
> +		if (ret)
> +			goto error_hw_stop;
> +	}

Because hid_hw_start is called earlier in the probe sequence, is it possible
for a hardware event to trigger lg_g710_event before these LEDs are actually
registered?

If the worker thread runs and invokes the brightness hardware changed
notification on zero-initialized, unregistered LED devices, won't the
internal sysfs nodes be NULL, triggering a stack trace warning in the LED
core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518153519.605627-1-xav@bes.tel?part=1