[PATCH v2 0/2] HID: sensor: custom: Fix fields lifetime issues

[PATCH v2 0/2] HID: sensor: custom: Fix fields lifetime issues

[Haoxiang Li]

Hi,

This series fixes lifetime issues around sensor_inst->fields and the
sysfs attributes that can access it.

The first patch fixes the remove path by removing enable_sensor before
freeing the field attributes. enable_sensor_store() can dereference
power_state and report_state, which point into sensor_inst->fields.

The second patch fixes the original field sysfs group leak on probe
failure. It creates the field attributes before exposing enable_sensor,
then unwinds any field groups that were created before a later
sysfs_create_group() failure.

Thanks, Jiri, for the review and for pointing out the UAF concern.

Changes in v2:
- Split the fix into two patches.
- Fix the pre-existing remove path ordering issue.
- Create field attributes before exposing enable_sensor.
- Unwind already-created field sysfs groups on failure.

Haoxiang Li (2):
  HID: sensor: custom: Remove enable_sensor before freeing fields
  HID: sensor: custom: Fix field sysfs group cleanup on failure

 drivers/hid/hid-sensor-custom.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)


base-commit: ef0c9f75a19532d7675384708fc8621e10850104
-- 
2.25.1

[PATCH v2 1/2] HID: sensor: custom: Remove enable_sensor before freeing fields

[Haoxiang Li]

enable_sensor_store() can call set_power_report_state(), which
dereferences sensor_inst->power_state and sensor_inst->report_state.
These pointers refer to entries in sensor_inst->fields.

hid_sensor_custom_remove() currently frees the field attributes before
removing the enable_sensor sysfs attribute, leaving a window where a
concurrent sysfs write can dereference freed memory.

Remove enable_sensor before freeing the field attributes.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
 drivers/hid/hid-sensor-custom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index afffea894021..d7bdbae96b50 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -1042,9 +1042,9 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 	}
 
 	hid_sensor_custom_dev_if_remove(sensor_inst);
-	hid_sensor_custom_remove_attributes(sensor_inst);
 	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
 			   &enable_sensor_attr_group);
+	hid_sensor_custom_remove_attributes(sensor_inst);
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
 }
 
-- 
2.25.1

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

[Haoxiang Li]

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.

Create the field attributes before exposing enable_sensor, so the
failure path can free sensor_inst->fields without leaving enable_sensor
able to access power_state or report_state pointers into that array.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
 drivers/hid/hid-sensor-custom.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index d7bdbae96b50..ea98088e5112 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -609,7 +609,7 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
 					 &sensor_inst->fields[i].
 					 hid_custom_attribute_group);
 		if (ret)
-			break;
+			goto err_remove_groups;
 
 		/* For power or report field store indexes */
 		if (sensor_inst->fields[i].attribute.attrib_id ==
@@ -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;
 }
 
 static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *
@@ -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;
 
 	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);
+err_remove_attributes:
+	hid_sensor_custom_remove_attributes(sensor_inst);
 err_remove_callback:
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
 
-- 
2.25.1

Re: [PATCH v2 1/2] HID: sensor: custom: Remove enable_sensor before freeing fields

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Out-of-bounds array access in `show_value()` and `store_value()` due to flawed sysfs attribute index parsing.
- [High] A race condition in `hid_sensor_custom_remove()` allows incoming sensor events to trigger a Use-After-Free.
- [High] Using `devm_kzalloc` for the driver context linked to a `miscdevice` causes a Use-After-Free if the device is unbound while the file is open.
- [Medium] Concurrent reads on the `miscdevice` corrupt the `kfifo` state, leading to broken data streams.
--

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

HID: sensor: custom: Remove enable_sensor before freeing fields

enable_sensor_store() can call set_power_report_state(), which
dereferences sensor_inst->power_state and sensor_inst->report_state.
These pointers refer to entries in sensor_inst->fields.

hid_sensor_custom_remove() currently frees the field attributes before
removing the enable_sensor sysfs attribute, leaving a window where a
concurrent sysfs write can dereference freed memory.

Remove enable_sensor before freeing the field attributes.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index afffea8940214..d7bdbae96b501 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -1042,9 +1042,9 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
>  	}
>  
>  	hid_sensor_custom_dev_if_remove(sensor_inst);
> -	hid_sensor_custom_remove_attributes(sensor_inst);
>  	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
>  			   &enable_sensor_attr_group);
> +	hid_sensor_custom_remove_attributes(sensor_inst);
>  	sensor_hub_remove_callback(hsdev, hsdev->usage);

[Severity: High]
This is a pre-existing issue, but does freeing the kfifo resource before
unregistering the interrupt-driven callback leave a window for a
use-after-free?

If hid_sensor_custom_dev_if_remove() frees sensor_inst->data_fifo, and an
incoming hardware event occurs before sensor_hub_remove_callback() is called,
could an open file descriptor trigger hid_sensor_capture_sample() to write to
the freed kfifo?

drivers/hid/hid-sensor-custom.c:hid_sensor_capture_sample() {
    ...
    if (kfifo_avail(&sensor_inst->data_fifo) >= required_size) {
        kfifo_in(&sensor_inst->data_fifo, ...);
    ...
}

>  }

[Severity: High]
This isn't a bug introduced by this patch, but it appears using devm_kzalloc()
for the driver context linked to a miscdevice might cause a use-after-free.

If the device is unbound (e.g., via USB unplug), devres frees sensor_inst.
However, if a user process holds the miscdevice file descriptor open, is it
possible for a subsequent call to read() to extract and dereference the freed
pointer?

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)) {
    ...
}

[Severity: Critical]
This is a pre-existing issue, but we might have an out-of-bounds array access
when parsing the attribute name for an index.

In hid_sensor_custom_add_field(), the raw HID loop index is baked into the
sysfs filename:

drivers/hid/hid-sensor-custom.c:hid_sensor_custom_add_field() {
    ...
    snprintf(sensor_field->group_name, sizeof(sensor_field->group_name),
             "input-%x-%x", sensor_field->attribute.index,
             sensor_field->attribute.attrib_id);
    ...
}

If fields are skipped and the array is reallocated to a smaller size, would a
user reading the sysfs file trigger an out-of-bounds access in show_value(),
given it uses the raw index directly from the filename?

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

[Severity: Medium]
This isn't a bug introduced by this patch, but can concurrent readers corrupt
the internal indices by invoking the kfifo consumer API without serialization?

While open() prevents multiple independent opens, a process can fork() or
spawn threads to invoke read() concurrently on the same file descriptor.

Since hid_sensor_custom_read() does not use a mutex around kfifo_to_user(),
will racing threads corrupt the kfifo state?

drivers/hid/hid-sensor-custom.c:hid_sensor_custom_read() {
    ...
    ret = kfifo_to_user(&sensor_inst->data_fifo, buf, count, &copied);
    ...
}

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

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

[sashiko-bot]

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

Re: [PATCH v2 1/2] HID: sensor: custom: Remove enable_sensor before freeing fields

[Jonathan Cameron]

On Thu,  2 Jul 2026 17:48:55 +0800
Haoxiang Li <haoxiang_li2024@163.com> wrote:

> enable_sensor_store() can call set_power_report_state(), which
> dereferences sensor_inst->power_state and sensor_inst->report_state.
> These pointers refer to entries in sensor_inst->fields.
> 
> hid_sensor_custom_remove() currently frees the field attributes before
> removing the enable_sensor sysfs attribute, leaving a window where a
> concurrent sysfs write can dereference freed memory.
> 
> Remove enable_sensor before freeing the field attributes.
> 
> Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>

If this is the UAF that Jiri called out in patch one, please
credit the bot with a Reported-by tag and link to that review.  Example:
https://lore.kernel.org/all/20260702183644.60827-1-adrian.hunter@intel.com/


> ---
>  drivers/hid/hid-sensor-custom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index afffea894021..d7bdbae96b50 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -1042,9 +1042,9 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
>  	}
>  
>  	hid_sensor_custom_dev_if_remove(sensor_inst);
> -	hid_sensor_custom_remove_attributes(sensor_inst);
>  	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
>  			   &enable_sensor_attr_group);
> +	hid_sensor_custom_remove_attributes(sensor_inst);

Given this is out of order with respect to reversing what happens in probe,
please add a comment to say why (and ensure no one fixes it back to
the original order!)

It may be that a reorder in probe is needed as well to bring things into
balance and ensure that fields is available when
that enable_sensor_attr_group is registered but I haven't analysed it closely.


>  	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  }
>  

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

[Jonathan Cameron]

On Thu,  2 Jul 2026 17:48:56 +0800
Haoxiang Li <haoxiang_li2024@163.com> wrote:

> 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.
> 
> Create the field attributes before exposing enable_sensor, so the
> failure path can free sensor_inst->fields without leaving enable_sensor
> able to access power_state or report_state pointers into that array.
> 
> Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
Ah. This has the reorder necessary to resolve the issue I called out
in previous patch (I think).  They can't be done independently.
Can you move the registration reorder to the previous patch then
only do the missing cleanup in this one?

> ---
>  drivers/hid/hid-sensor-custom.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index d7bdbae96b50..ea98088e5112 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -609,7 +609,7 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
>  					 &sensor_inst->fields[i].
>  					 hid_custom_attribute_group);
>  		if (ret)
> -			break;
> +			goto err_remove_groups;
>  
>  		/* For power or report field store indexes */
>  		if (sensor_inst->fields[i].attribute.attrib_id ==
> @@ -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;
>  }
>  
>  static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *
> @@ -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);

I think this brings things back into order wrt to remove.
This probably needs to be in the previous patch.

>  	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;
>  
>  	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);
> +err_remove_attributes:
> +	hid_sensor_custom_remove_attributes(sensor_inst);
>  err_remove_callback:
>  	sensor_hub_remove_callback(hsdev, hsdev->usage);
>