Re: [PATCH v6 1/3] HID: lamparray: add new LampArray helper module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] hid: lamparray: out-of-bounds access on field->value array
- [Medium] hid: lamparray: mixing goto and scope-based cleanup
- [Medium] hid: lamparray: silent state update failures due to unverified shared reports
- [Medium] hid: lamparray: TOCTOU race in unregister leaves hardware in wrong state
- [Medium] hid: lamparray: error path fails to revert autonomous mode
- [Medium] hid: lamparray: data race on LED class device state
- [Medium] hid: lamparray: initialization race due to early sysfs exposure
- [Low] leds: lamparray: prints success log messages
--

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

HID: lamparray: add new LampArray helper module

Add a new hid-lamparray helper module that provides basic support for
devices exposing a Lighting/LampArray application collection. It registers a
single-zone RGB LED representation via the LED subsystem.

> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> --- /dev/null
> +++ b/drivers/hid/hid-lamparray.c

[ ... ]

> +static int lamparray_read_attributes_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +	int ret;
> +	size_t len;
> +
> +	if (!ldev->lamp_count.field) {
> +		hid_warn(hdev, "No LampCount field found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!ldev->lamparray_kind.field) {
> +		hid_warn(hdev, "No LampArrayKind field found\n");
> +		return -ENODEV;
> +	}
> +
> +	report = ldev->lamp_count.field->report;
> +
> +	if (!report) {

[Severity: Medium]
Can lamparray_kind read stale or zeroed memory here? By only fetching the
report associated with lamp_count, the lamparray_kind field may not be
updated if it resides in a different HID report than lamp_count.

> +		hid_warn(hdev, "LampCount field has no report\n");
> +		return -ENODEV;
> +	}

[ ... ]

> +static int lamparray_parse_update_report(struct lamparray_device *ldev)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report_enum *re;
> +	struct hid_report *report;
> +	struct hid_field *field;
> +	int i, j;
> +	int ret = 0;
> +
> +	re = &hdev->report_enum[HID_FEATURE_REPORT];
> +
> +	list_for_each_entry(report, &re->report_list, list) {
> +		for (i = 0; i < report->maxfield; i++) {
> +			field = report->field[i];
> +			if (!field)
> +				continue;
> +
> +			if (!field->usage || !field->maxusage)
> +				continue;
> +
> +			for (j = 0; j < field->maxusage; j++) {
> +				u32 usage = field->usage[j].hid;

[Severity: High]
Could this loop overflow the field->value array? The loop iterates up to
field->maxusage, storing the loop index in the field_value structs.
However, the field->value array is allocated by the HID core based on
field->report_count. If a crafted HID descriptor specifies a maxusage
larger than report_count, the subsequent reads and writes in
get_field_value() and set_field_value() can overflow the array.

> +				u32 collection_idx = field->usage[j].collection_index;
> +				u32 collection_usage = hdev->collection[collection_idx].usage;

[ ... ]

> +static int lamparray_hw_set_state(struct lamparray_device *ldev, u8 r, u8 g,
> +				  u8 b, u8 intensity)
> +{
> +	struct hid_device *hdev = ldev->hdev;
> +	struct hid_report *report;
> +
> +	if (!lamparray_color_fields_is_complete(&ldev->color_update))
> +		return -ENODEV;
> +
> +	if (ldev->range_start.field && ldev->range_end.field) {
> +		set_field_value(&ldev->range_start, 0);
> +		set_field_value(&ldev->range_end, ldev->lamp_count_value - 1);
> +	}
> +
> +	set_field_value(&ldev->color_update.red, r);
> +	set_field_value(&ldev->color_update.green, g);
> +	set_field_value(&ldev->color_update.blue, b);
> +	set_field_value(&ldev->color_update.intensity, intensity);
> +
> +	report = ldev->color_update.red.field->report;
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> +
> +	return 0;

[Severity: Medium]
Does this drop updates for the other color channels and range fields? If
green, blue, intensity, or the range fields reside in a different report
than the red field, sending only color_update.red.field->report will skip
their updates.

> +}

[ ... ]

> +static int lamparray_restore_state(struct lamparray_device *ldev)
> +{
> +	u8 r, g, b;
> +	int ret;
> +	enum led_brightness brightness;
> +
> +	if (!ldev->use_leds_uapi)
> +		return 0;
> +
> +	guard(mutex)(&ldev->dev_lock);
> +
> +	r = ldev->last_r;
> +	g = ldev->last_g;
> +	b = ldev->last_b;
> +	brightness = ldev->last_brightness;
> +
> +	ldev->mc_cdev.subled_info[0].intensity = r;
> +	ldev->mc_cdev.subled_info[1].intensity = g;
> +	ldev->mc_cdev.subled_info[2].intensity = b;
> +	ldev->mc_cdev.led_cdev.brightness = brightness;

[Severity: Medium]
Can these updates race with concurrent sysfs writes? Modifying these LED
class device fields without holding the led_cdev->led_access mutex while
the sysfs interface is active can cause inconsistent brightness or color
states if userspace writes to them simultaneously.

> +
> +	/*
> +	 * Quirk for devices ignoring intensity values, see
> +	 * lamparray_led_brightness_set for more information.
> +	 */

[ ... ]

> +static ssize_t use_leds_uapi_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct lamparray_device *ldev = lamparray_ldev_from_sysfs_dev(dev);
> +	bool val, old_val;
> +	int ret;
> +
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Make sure only one concurrent write is possible */
> +	guard(mutex)(&ldev->sysfs_lock);
> +	old_val = ldev->use_leds_uapi;
> +
> +	if (val == old_val)
> +		return count;
> +
> +	ldev->use_leds_uapi = val;
> +
> +	/*
> +	 * Take the device out of autonomous mode before exposing the LED
> +	 * class device, and hand it back afterwards, so that control is
> +	 * transferred in both directions rather than only at probe.
> +	 */
> +	if (val == 1) {
> +		ret = lamparray_hw_set_autonomous(ldev, false);
> +		if (ret)
> +			goto err_revert;

[Severity: Medium]
Does this function mix goto-based error handling with scope-based cleanup?
The cleanup subsystem guidelines request converting all resources to
scope-based cleanup if using __free() or guard(). Mixing the paradigms in
functions like lamparray_register() and use_leds_uapi_store() can create
ownership semantics confusion.

> +		ret = lamparray_register_led(ldev);
> +		if (ret) {
> +			lamparray_hw_set_autonomous(ldev, true);
> +			goto err_revert;

[ ... ]

> +struct lamparray *
> +lamparray_register(struct hid_device *hdev,
> +		   const struct lamparray_init_state *led_init_state)
> +{
> +	int ret;
> +	struct lamparray_device *ldev;
> +
> +	if (!hdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	struct lamparray *la __free(kfree) = kzalloc_obj(*la, GFP_KERNEL);
> +	if (!la)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ldev = &la->ldev;

[ ... ]

> +	ret = xa_err(xa_store(&lamparray_by_hdev, (unsigned long)hdev, ldev,
> +			      GFP_KERNEL));
> +	if (ret)
> +		goto err_unregister_led;
> +
> +	ret = lamparray_register_sysfs(ldev);
> +	if (ret)
> +		goto err_xa_erase;
> +
> +	ret = lamparray_hw_set_autonomous(ldev, false);

[Severity: Medium]
Could exposing the sysfs file before hardware initialization completes
create a race condition? If userspace writes 0 to use_leds_uapi right
after this call, the store handler will unregister the LED and set
autonomous to true. This probe thread then resumes, unconditionally setting
autonomous to false and skipping lamparray_restore_state(), leaving the
device in a broken state.

> +	if (ret) {
> +		hid_err(hdev, "Could not disable autonomous mode: %d", ret);
> +		goto err_remove_sysfs;
> +	}
> +
> +	hid_info(hdev, "LampArray device registered\n");

[Severity: Low]
Should this log message be removed? The LED subsystem guidelines specify
that drivers should only log errors or warnings, avoiding success messages
on probe.

> +
> +	ret = lamparray_restore_state(ldev);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set default state: %d", ret);
> +		goto err_remove_sysfs;
> +	}

[Severity: Medium]
Does this error path leave the hardware in a non-autonomous state? If
lamparray_restore_state() fails, the code jumps to err_remove_sysfs and
cleans up software state, but does not revert the device back to autonomous
mode.

> +
> +	return_ptr(la);
> +
> +err_remove_sysfs:
> +	lamparray_remove_sysfs(ldev);
> +err_xa_erase:
> +	xa_erase(&lamparray_by_hdev, (unsigned long)hdev);

[ ... ]

> +void lamparray_unregister(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	lamparray_hw_set_autonomous(ldev, true);
> +
> +	lamparray_remove_sysfs(ldev);

[Severity: Medium]
Can this sequence permanently leave the hardware in a non-autonomous state?
Calling lamparray_hw_set_autonomous(..., true) before removing the sysfs
interface creates a window where a concurrent sysfs write to use_leds_uapi
can switch the device back to non-autonomous mode right before the device
is completely unregistered.

> +	xa_erase(&lamparray_by_hdev, (unsigned long)ldev->hdev);
> +
> +	mutex_lock(&ldev->sysfs_lock);
> +	lamparray_unregister_led(ldev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916144838.456239-1-aer@tuxedocomputers.com?part=1