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

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 <[email protected]>

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/[email protected]?part=1