[PATCH v2 RESEND] HID: sensor-hub: Fix out-of-bounds write in sensor_hub_get_feature

[PATCH v2 RESEND] HID: sensor-hub: Fix out-of-bounds write in sensor_hub_get_feature

[bakabaka9]

From: Xingrui Li <baka9@bakabaka9.tech>

sensor_hub_get_feature() clamps its return value to the caller's buffer
size, but the copy loop still copies field->report_size / 8 bytes for
each report value. A malicious HID descriptor can advertise a large
feature field size while an IIO caller supplies a small stack buffer,
such as a single s32, causing an out-of-bounds write.

HID core stores parsed report values in __s32 slots and clamps extracted
values to 32 bits. Reject feature fields that require more than one slot
per value, guard the total byte count calculation, and clamp each
per-value copy to the remaining caller buffer.

Fixes: 5459ada2b3cd69 ("HID: sensor-hub: Fix packing of result buffer for feature report")
Cc: stable@kernel.org
Assisted-by: OpenAI:GPT-5.5-Cyber
Signed-off-by: Xingrui Li <baka9@bakabaka9.tech>
---
 drivers/hid/hid-sensor-hub.c | 42 +++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 34f710c465b8..6470a290ebfc 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -239,12 +239,17 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
 			   u32 field_index, int buffer_size, void *buffer)
 {
 	struct hid_report *report;
+	struct hid_field *field;
 	struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
-	int report_size;
+	size_t field_size;
+	size_t report_size;
+	size_t copied = 0;
+	size_t to_copy;
 	int ret = 0;
-	u8 *val_ptr;
-	int buffer_index = 0;
-	int i;
+	unsigned int i;
+
+	if (!buffer || buffer_size <= 0)
+		return -EINVAL;
 
 	memset(buffer, 0, buffer_size);
 
@@ -258,26 +263,29 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
 	hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
 	hid_hw_wait(hsdev->hdev);
 
+	field = report->field[field_index];
+
 	/* calculate number of bytes required to read this field */
-	report_size = DIV_ROUND_UP(report->field[field_index]->report_size,
-				   8) *
-				   report->field[field_index]->report_count;
-	if (!report_size) {
+	field_size = DIV_ROUND_UP(field->report_size, 8);
+	/* HID core stores each parsed report value in a __s32 slot. */
+	if (!field_size || field_size > sizeof(field->value[0])) {
+		ret = -EINVAL;
+		goto done_proc;
+	}
+	if (field->report_count > SIZE_MAX / field_size) {
 		ret = -EINVAL;
 		goto done_proc;
 	}
-	ret = min(report_size, buffer_size);
 
-	val_ptr = (u8 *)report->field[field_index]->value;
-	for (i = 0; i < report->field[field_index]->report_count; ++i) {
-		if (buffer_index >= ret)
-			break;
+	report_size = field_size * field->report_count;
+	report_size = min_t(size_t, report_size, buffer_size);
 
-		memcpy(&((u8 *)buffer)[buffer_index], val_ptr,
-		       report->field[field_index]->report_size / 8);
-		val_ptr += sizeof(__s32);
-		buffer_index += (report->field[field_index]->report_size / 8);
+	for (i = 0; i < field->report_count && copied < report_size; ++i) {
+		to_copy = min(field_size, report_size - copied);
+		memcpy(&((u8 *)buffer)[copied], &field->value[i], to_copy);
+		copied += to_copy;
 	}
+	ret = copied;
 
 done_proc:
 	mutex_unlock(&data->mutex);
-- 
2.43.0

Re: [PATCH v2 RESEND] HID: sensor-hub: Fix out-of-bounds write in sensor_hub_get_feature

[sashiko-bot]

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

Pre-existing issues:
- [High] Both `sensor_hub_get_feature` and `sensor_hub_set_feature` improperly handle endianness, causing silent data corruption when interfacing with sensor hubs on Big-Endian architectures.
--

--- Patch [1]: [PATCH v2 RESEND] HID: sensor-hub: Fix out-of-bounds write in sensor_hub_get_feature ---
commit 75b1fca160b481a6f3fa0937e0916de1a229cdf0
Author: Xingrui Li <baka9@bakabaka9.tech>

HID: sensor-hub: Fix out-of-bounds write in sensor_hub_get_feature

This commit resolves an out-of-bounds write vulnerability in
sensor_hub_get_feature() by validating the feature field size and clamping
the per-value copy to the remaining capacity of the caller-supplied buffer.

> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 90666ff629def..9bbda38ded968 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c

[ ... ]

> @@ -258,26 +263,29 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>  	hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
>  	hid_hw_wait(hsdev->hdev);
>  
> +	field = report->field[field_index];
> +
>  	/* calculate number of bytes required to read this field */
> -	report_size = DIV_ROUND_UP(report->field[field_index]->report_size,
> -				   8) *
> -				   report->field[field_index]->report_count;
> -	if (!report_size) {
> +	field_size = DIV_ROUND_UP(field->report_size, 8);
> +	/* HID core stores each parsed report value in a __s32 slot. */
> +	if (!field_size || field_size > sizeof(field->value[0])) {
> +		ret = -EINVAL;
> +		goto done_proc;
> +	}
> +	if (field->report_count > SIZE_MAX / field_size) {
>  		ret = -EINVAL;
>  		goto done_proc;
>  	}
> -	ret = min(report_size, buffer_size);
>  
> -	val_ptr = (u8 *)report->field[field_index]->value;
> -	for (i = 0; i < report->field[field_index]->report_count; ++i) {
> -		if (buffer_index >= ret)
> -			break;
> +	report_size = field_size * field->report_count;
> +	report_size = min_t(size_t, report_size, buffer_size);
>  
> -		memcpy(&((u8 *)buffer)[buffer_index], val_ptr,
> -		       report->field[field_index]->report_size / 8);
> -		val_ptr += sizeof(__s32);
> -		buffer_index += (report->field[field_index]->report_size / 8);
> +	for (i = 0; i < field->report_count && copied < report_size; ++i) {
> +		to_copy = min(field_size, report_size - copied);
> +		memcpy(&((u8 *)buffer)[copied], &field->value[i], to_copy);

[Severity: High]
This is a pre-existing issue, but does this memcpy() cause silent data 
corruption on Big-Endian architectures?

HID core extracts report bits into a native CPU-endian __s32 format, but
this memcpy() fetches the lowest-addressed bytes. On Big-Endian platforms,
the lowest memory address of the native integer contains the most significant
bits, which are zero-padding for fields smaller than 32 bits. Will this copy
padding bytes instead of the actual data?

Similarly, in sensor_hub_set_feature():

drivers/hid/hid-sensor-hub.c:sensor_hub_set_feature() {
	...
	ret = hid_set_field(report->field[field_index], i,
			    (__force __s32)cpu_to_le32(*buf32));
	...
}

Does converting the native integer to Little-Endian before passing it to
hid_set_field() break Big-Endian architectures? It looks like HID core
expects a native CPU-endian value to correctly extract the least significant
bits to populate the little-endian HID report byte stream.

> +		copied += to_copy;
>  	}
> +	ret = copied;
>  
>  done_proc:
>  	mutex_unlock(&data->mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805185753.3069-1-baka9@bakabaka9.tech?part=1