Re: [PATCH] HID: sensor-hub: fix out-of-bounds access in sensor_hub_get_feature()

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

Pre-existing issues:
- [High] The introduced bounds check allows reading adjacent independent items into a single item's buffer space when `report_size > 32`.
- [High] Byte-wise `memcpy` from native-endian `s32` array copies padding bytes on Big-Endian architectures for fields smaller than 32 bits.
- [High] Fields smaller than 8 bits are silently dropped due to integer division by 8.
--

--- Patch [1]: [PATCH] HID: sensor-hub: fix out-of-bounds access in sensor_hub_get_feature() ---
commit b3260de30fa032d167e378c546af0b283ff6ce04
Author: Shengzhuo Wei <[email protected]>

HID: sensor-hub: fix out-of-bounds access in sensor_hub_get_feature()

This commit addresses an out-of-bounds access issue in
sensor_hub_get_feature() by bounding the per-iteration copy size. It restricts
the copy size to the minimum of the report size, remaining source buffer, and
remaining destination buffer.

> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 90666ff629def..7e97ca98a3883 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -270,11 +270,18 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>  
>  	val_ptr = (u8 *)report->field[field_index]->value;
>  	for (i = 0; i < report->field[field_index]->report_count; ++i) {
> +		int copy = report->field[field_index]->report_size / 8;

[Severity: High]
This is a pre-existing issue, but does this silently drop fields smaller
than 8 bits?

When sensor_hub_get_feature() is called for a sub-byte field (like a 4-bit
flag), the integer division by 8 evaluates to 0. This causes memcpy to copy
0 bytes, leaving the caller's buffer unpopulated even though the HID core
correctly parsed the value into the field->value array.

> +		int src_remaining = (report->field[field_index]->report_count - i) *
> +				    sizeof(__s32);
> +
>  		if (buffer_index >= ret)
>  			break;
>  
> -		memcpy(&((u8 *)buffer)[buffer_index], val_ptr,
> -		       report->field[field_index]->report_size / 8);
> +		if (copy > src_remaining)
> +			copy = src_remaining;

[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check still
allow reading adjacent independent items into a single item's buffer space?

The HID core truncates and stores each field item in a single 4-byte __s32
element. By bounding the copy size to src_remaining (the remaining size of
the entire field->value array) instead of a single element (sizeof(__s32)),
if the report size is greater than 32 bits, the copy size will exceed
4 bytes.

Could this cause memcpy to incorrectly read the next independent value
element from the array and append it to the current item?

> +		if (copy > buffer_size - buffer_index)
> +			copy = buffer_size - buffer_index;
> +		memcpy(&((u8 *)buffer)[buffer_index], val_ptr, copy);

[Severity: High]
This is a pre-existing issue, but does this byte-wise memcpy from a
native-endian s32 array copy padding bytes on big-endian architectures for
fields smaller than 32 bits?

The function extracts fields that the HID core has parsed into native-endian
32-bit integers. For a 16-bit field on a big-endian system, the valid data
resides at byte offsets 2 and 3, but memcpy copies from the lowest memory
address.

Could this result in returning zeroes by copying the most significant
padding bytes instead of the actual data?

>  		val_ptr += sizeof(__s32);
>  		buffer_index += (report->field[field_index]->report_size / 8);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1