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

[sashiko-bot@kernel.org]

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