Re: [PATCH v7 2/3] HID: generic: add LampArray support via hid-lamparray helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] HID: generic: drvdata clobber and uninitialized read in hid_generic_remove()
- [High] HID: generic: double hid_hw_start() in hid_generic_probe() corrupts reports
--

commit 977659b4786943b7473279419965b0035b45922c
Author: Aaron Erhardt <aer@tuxedocomputers.com>

HID: generic: add LampArray support via hid-lamparray helper

This commit checks for LampArray support in hid_generic_probe() after
hid_parse() and registers a lamparray instance when present. On failure, it
stops the hardware and falls back to starting the device without LampArray,
and unregisters the lamparray instance in hid_generic_remove().

Link: https://lore.kernel.org/all/20260728115918.125349-2-aer@tuxedocomputers.com

[ ... ]

> diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
> index c2de916747dec..fde772c5014bb 100644
> --- a/drivers/hid/hid-generic.c
> +++ b/drivers/hid/hid-generic.c

[ ... ]

> @@ -67,6 +69,34 @@ static int hid_generic_probe(struct hid_device *hdev,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Optional: attach LampArray support if present.
> +	 * Never fail probe on LampArray errors; keep device functional.
> +	 */
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && lamparray_is_supported_device(hdev)) {
> +		/*
> +		 * Use HID_CONNECT_DRIVER to make sure requests are
> +		 * processed in hid_report_raw_event. Without this,
> +		 * hid-core will skip parsing for devices that are only
> +		 * claimed by hidraw, thus making it impossible to query
> +		 * the required information.
> +		 * See 7704ac937345 for more information about this flag
> +		 * and why it is necessary.
> +		 */
> +		ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
> +		if (ret)
> +			return ret;
> +
> +		la = lamparray_register(hdev, NULL);
> +		if (IS_ERR(la)) {
> +			hid_hw_stop(hdev);
> +			hid_warn(hdev, "LampArray init failed: %ld\n", PTR_ERR(la));
> +		} else {
> +			hid_set_drvdata(hdev, la);
> +			return 0;
> +		}
> +	}
> +
>  	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  }

[Severity: High]
Does calling hid_hw_start() a second time on the fallback path when
lamparray_register() fails leak report->field_entries and duplicate entries
in report->field_entry_list?

The first hid_hw_start() calls hid_connect() -> hid_process_ordering() ->
hid_report_process_ordering() for every HID_INPUT_REPORT:

drivers/hid/hid-core.c:hid_report_process_ordering() {
	...
	/* allocate the memory to process the fields */
	entries = kzalloc_objs(*entries, count);
	if (!entries)
		return;

	report->field_entries = entries;
	...
}

And appends each entry to report->field_entry_list (which was initialized
once during hid_parse() -> hid_register_report()).

When lamparray_register() fails, hid_generic_probe() calls hid_hw_stop() ->
hid_disconnect(), which does not free report->field_entries or reinitialize
report->field_entry_list because those are only cleaned up in
hid_close_report() -> hid_free_report():

drivers/hid/hid-core.c:hid_free_report() {
	unsigned n;

	kfree(report->field_entries);

	for (n = 0; n < report->maxfield; n++)
		kvfree(report->field[n]);
	kfree(report);
}

When hid_generic_probe() falls through to the second hid_hw_start() on the
same parsed reports, hid_report_process_ordering() allocates a new entries
array, overwrites report->field_entries without freeing the previous array,
and links a second set of entries into report->field_entry_list alongside
the still-linked entries from the first leaked array.

Wouldn't this permanently leak the first report->field_entries allocation and
cause hid_process_report() to process every input field twice per report?

> @@ -78,6 +108,16 @@ static int hid_generic_reset_resume(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static void hid_generic_remove(struct hid_device *hdev)
> +{
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_unregister(la);
> +
> +	hid_hw_stop(hdev);
> +}

[Severity: Critical]
Can using hid_set_drvdata() and hid_get_drvdata() here collide with low-level
HID transport drivers that store their own private data in
hdev->dev.driver_data?

For example, drivers/hid/hid-hyperv.c sets drvdata before registering the HID
device:

drivers/hid/hid-hyperv.c:mousevsc_probe() {
	...
	hid_set_drvdata(hid_dev, device);

	ret = hid_add_device(hid_dev);
	...
}

And drivers/platform/x86/tuxedo/nb04/wmi_ab.c sets drvdata inside its start
callback during hid_hw_start():

drivers/platform/x86/tuxedo/nb04/wmi_ab.c:tux_ll_start() {
	...
	dev_set_drvdata(&hdev->dev, driver_data);

	return ret;
}

Because hid_generic_probe() only calls hid_set_drvdata(hdev, la) when
lamparray_register() succeeds and leaves hdev->dev.driver_data untouched on
all other paths, if a device on such a transport does not support LampArray
or fails lamparray_register(), won't hid_generic_remove() retrieve the
transport driver's pointer via hid_get_drvdata(hdev) and pass it to
lamparray_unregister(la), corrupting memory and calling kfree() on the
transport driver's structure?

Conversely, if lamparray_register() succeeds on tuxedo_nb04_wmi, won't
hid_set_drvdata(hdev, la) overwrite the struct tux_hdev_driver_data_t pointer
stored by tux_ll_start(), causing subsequent tux_ll_raw_request() calls to
dereference struct lamparray as struct tux_hdev_driver_data_t?

[ ... ]

>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-hid-lamparray-v7-0-47418b667b47@tuxedocomputers.com?part=2