Re: [PATCH v2 2/2] HID: sensor: custom: Fix field sysfs group cleanup on failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lack of synchronization during callback removal leads to Use-After-Free of `sensor_inst` in HID event handler.
- [High] Active file descriptors on the misc device cause Use-After-Free after driver unbind.
- [High] Out-of-bounds array access in sysfs `show_value` and `store_value` due to flawed index parsing.
- [Medium] Resource leak of HID sensor hub device open count on unbind.
--

commit 33ce389be4baf215143f759dd6b8350c60263845
Author: Haoxiang Li <haoxiang_li2024@163.com>

HID: sensor: custom: Fix field sysfs group cleanup on failure

hid_sensor_custom_add_attributes() creates one sysfs group for each
custom sensor field. If sysfs_create_group() fails after some groups
have already been created, the function returns the error without
removing the previously created groups. Add a local unwind path to
remove the groups that were already created.

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index d7bdbae96b501..ea98088e51123 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c

[ ... ]

> @@ -621,6 +621,13 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
>  	}
>  
>  	return ret;
> +
> +err_remove_groups:
> +	while (--i >= 0)
> +		sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
> +				   &sensor_inst->fields[i].hid_custom_attribute_group);
> +	kfree(sensor_inst->fields);
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but can the sysfs read/write handlers access
out of bounds memory if fields are skipped during initialization?

When sysfs attributes are read or written, the original index is extracted
via sscanf() and used directly on sensor_inst->fields:

drivers/hid/hid-sensor-custom.c:show_value() {
    ...
    if (sscanf(attr->attr.name, "input-%x-%x-%s", &index, &usage, name) == 3) {
        input = true;
        field_index = index;
    }
    ...
    attribute = &sensor_inst->fields[field_index].attribute;
}

If the initialization loop skips fields, the extracted index can exceed the
array bounds of sensor_inst->fields, leading to an out-of-bounds access.

> @@ -1005,26 +1012,26 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> -				 &enable_sensor_attr_group);
> +	ret = hid_sensor_custom_add_attributes(sensor_inst);
>  	if (ret)
>  		goto err_remove_callback;
>  
> -	ret = hid_sensor_custom_add_attributes(sensor_inst);
> +	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> +				 &enable_sensor_attr_group);
>  	if (ret)
> -		goto err_remove_group;
> +		goto err_remove_attributes;
>  
>  	ret = hid_sensor_custom_dev_if_add(sensor_inst);
>  	if (ret)
> -		goto err_remove_attributes;
> +		goto err_remove_group;

[Severity: High]
This is a pre-existing issue, but does the misc device teardown properly
synchronize with active file descriptors?

When the driver unbinds or fails probe, it unregisters the misc device and
frees the data FIFO, but active file descriptors are not revoked.

If a userspace process holds an open file descriptor, can subsequent calls
dereference the freed sensor_inst memory?

drivers/hid/hid-sensor-custom.c:hid_sensor_custom_read() {
    ...
    sensor_inst = container_of(file->private_data,
                               struct hid_sensor_custom, custom_dev);
    if (kfifo_is_empty(&sensor_inst->data_fifo)) {
    ...
}

>  
>  	return 0;
>  
> -err_remove_attributes:
> -	hid_sensor_custom_remove_attributes(sensor_inst);
>  err_remove_group:
>  	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
>  			   &enable_sensor_attr_group);

[Severity: Medium]
This is a pre-existing issue, but is there a missing teardown for the sensor
hub open count?

If a user writes 1 to the enable_sensor attribute, it increments the hub's
ref_cnt. When the driver unbinds, hid_sensor_custom_remove() destroys the
sysfs groups but does not seem to check sensor_inst->enable or call
sensor_hub_device_close().

Does this leak the reference count and leave the hardware in a persistent
power state?

> +err_remove_attributes:
> +	hid_sensor_custom_remove_attributes(sensor_inst);
>  err_remove_callback:
>  	sensor_hub_remove_callback(hsdev, hsdev->usage);

[Severity: High]
This is a pre-existing issue, but does removing the callback here leave a
race condition with concurrent HID events?

drivers/hid/hid-sensor-hub.c:sensor_hub_remove_callback() removes the
callback from the list and frees it:

sensor_hub_remove_callback() {
    ...
    list_del(&callback->list);
    kfree(callback);
    ...
}

It doesn't synchronize with concurrent executions. If the driver fails probe
or is unbound, sensor_inst is freed via devres. Can this concurrently
dereference the freed sensor_inst when executing the callback?

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
    ...
    callback = sensor_hub_get_callback(hdev, ...);
    if (callback->capture_sample) {
        callback->capture_sample(hsdev, ..., callback->pdev);
    }
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702094856.1105555-1-haoxiang_li2024@163.com?part=2