[PATCH v2] HID: lg4ff: validate report length before fixed offsets

[PATCH v2] HID: lg4ff: validate report length before fixed offsets

[Jiancheng Huang]

lg4ff_raw_event() rewrites fixed report offsets when combined pedals are
enabled. It currently assumes that each product report contains every
source and destination byte used by the rewrite.

Return without rewriting a short report before each product-specific
access. Apply the same bound to the computed offset path.

Fixes: c832f86effbc ("HID: hid-logitech: Add combined pedal support Logitech wheels")
Signed-off-by: Jiancheng Huang <jchuang@seu.edu.cn>
Assisted-by: Codex:gpt-5.6-luna
---
Changes in v2:
- Add the relevant public mailing lists to Cc; no source changes.

Evidence (v7.2-rc4 KUnit/KASAN oracle under bounded QEMU):
Source: confirmed/hid_lg4ff_short_report_oob_raw_excerpt.log

[    2.638792] KTAP version 1
[    2.638888] 1..1
[    2.641100]     KTAP version 1
[    2.641235]     # Subtest: hid-lg4ff-exploration
[    2.641784]     # module: hid_logitech
[    2.641962]     1..1
[    2.644581] ==================================================================
[    2.645008] BUG: KASAN: slab-out-of-bounds in lg4ff_raw_event+0x2fd/0x410
[    2.645369] Read of size 1 at addr ffff88800229fc63 by task kunit_try_catch/67
[    2.645369] 
[    2.645369] CPU: 1 UID: 0 PID: 67 Comm: kunit_try_catch Tainted: G                 N  7.2.0-rc4-dirty #3 PREEMPT(full) 
[    2.645369] Tainted: [N]=TEST
[    2.645369] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[    2.645369] Call Trace:
[    2.645369]  <TASK>
[    2.645369]  dump_stack_lvl+0x53/0x70
[    2.645369]  print_report+0xd0/0x630
[    2.645369]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[    2.645369]  ? lg4ff_raw_event+0x2fd/0x410
[    2.645369]  kasan_report+0xe5/0x120
[    2.645369]  ? lg4ff_raw_event+0x2fd/0x410
[    2.645369]  lg4ff_raw_event+0x2fd/0x410
[    2.645369]  lg4ff_short_report_oob_test+0x14e/0x250

 drivers/hid/hid-lg4ff.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index e901fdb7d..62f99a932 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -336,6 +336,8 @@ int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report,
 	if (entry->wdata.combine) {
 		switch (entry->wdata.product_id) {
 		case USB_DEVICE_ID_LOGITECH_WHEEL:
+			if (size < 7)
+				return 0;
 			rd[5] = rd[3];
 			rd[6] = 0x7F;
 			return 1;
@@ -343,10 +345,14 @@ int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report,
 		case USB_DEVICE_ID_LOGITECH_WINGMAN_FFG:
 		case USB_DEVICE_ID_LOGITECH_MOMO_WHEEL:
 		case USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2:
+			if (size < 6)
+				return 0;
 			rd[4] = rd[3];
 			rd[5] = 0x7F;
 			return 1;
 		case USB_DEVICE_ID_LOGITECH_DFP_WHEEL:
+			if (size < 7)
+				return 0;
 			rd[5] = rd[4];
 			rd[6] = 0x7F;
 			return 1;
@@ -366,6 +372,8 @@ int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report,
 		}
 
 		/* Compute a combined axis when wheel does not supply it */
+		if (size <= offset + 1)
+			return 0;
 		rd[offset] = (0xFF + rd[offset] - rd[offset+1]) >> 1;
 		rd[offset+1] = 0x7F;
 		return 1;
-- 
2.43.0

Re: [PATCH v2] HID: lg4ff: validate report length before fixed offsets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Force feedback is initialized on the input device after it has already been registered and made visible to userspace, violating input subsystem initialization order.
- [High] Use-after-free of the lg4ff device properties (`entry`) during device removal or probe failure, as the properties are freed before hardware I/O and input devices are fully unregistered.
- [Low] Data race on `entry->wdata.combine` between sysfs store and interrupt context raw event handler.
--

--- Patch [1]: [PATCH v2] HID: lg4ff: validate report length before fixed offsets ---
commit 64ee3d21ef774099318a4223423dc4d5d4eac310
Author: Jiancheng Huang <jchuang@seu.edu.cn>

HID: lg4ff: validate report length before fixed offsets
    
lg4ff_raw_event() rewrites fixed report offsets when combined pedals are
enabled. It currently assumes that each product report contains every
source and destination byte used by the rewrite.
    
Return without rewriting a short report before each product-specific
access. Apply the same bound to the computed offset path.


[Severity: High]
This isn't a bug introduced by this patch, but force feedback is initialized on
the input device after it has already been registered.

Looking at drivers/hid/hid-lg.c, lg_probe() registers the input device before
force feedback capabilities are set up:

drivers/hid/hid-lg.c:lg_probe() {
    ...
	ret = hid_hw_start(hdev, connect_mask);
    ...
	if (drv_data->quirks & LG_FF4)
		ret = lg4ff_init(hdev);
    ...
}

Because hid_hw_start() exposes the device to userspace, could userspace (e.g.
udev, games) open the device and observe an incomplete capability set before
lg4ff_init() modifies dev->ffbit and installs the FF callbacks?


[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk for the lg4ff
device properties during device removal?

In drivers/hid/hid-lg.c, the device properties are freed before hardware I/O
and input devices are fully unregistered:

drivers/hid/hid-lg.c:lg_remove() {
	struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
	if (drv_data->quirks & LG_FF4)
		lg4ff_deinit(hdev);
	hid_hw_stop(hdev);
	kfree(drv_data);
}

Since lg4ff_deinit() sets drv_data->device_props = NULL and frees the entry,
if an interrupt fires before hid_hw_stop() executes, could lg4ff_raw_event()
cache the entry pointer just before it is freed, leading to a use-after-free?

Additionally, could lg4ff_play() be concurrently invoked via userspace ioctl
while the input device is still registered, resulting in a similar
use-after-free?


> diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
> index e901fdb7d033c..62f99a93210c9 100644
> --- a/drivers/hid/hid-lg4ff.c
> +++ b/drivers/hid/hid-lg4ff.c
> @@ -336,6 +336,8 @@ int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	if (entry->wdata.combine) {

[Severity: Low]
This is a pre-existing issue, but does reading entry->wdata.combine from
interrupt context without locks or READ_ONCE() introduce a data race?

The sysfs store function (lg4ff_combine_store) writes to this shared boolean
state without locks or WRITE_ONCE(). Could this lead to a transient read of an
inconsistent state if a user writes to the sysfs node while the device is
actively sending events?

>  		switch (entry->wdata.product_id) {
>  		case USB_DEVICE_ID_LOGITECH_WHEEL:
> +			if (size < 7)
> +				return 0;
>  			rd[5] = rd[3];

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724151317.1419574-1-jchuang@seu.edu.cn?part=1