Re: [PATCH 1/2] HID: sensor-hub: Add sensor_hub_input_attr_read_values() for multi-byte reads

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Missing spinlock acquisition in the timeout/abort path of `sensor_hub_input_attr_read_values` leads to race conditions, use-after-free, and stack memory corruption.

Pre-existing issues:
- [High] Reading past the end of short HID reports exposes uninitialized kernel heap memory.
- [High] Repeated field matches in `sensor_hub_raw_event()` cause uncontrolled `kmemdup()` allocations, leaking memory.
--

commit b3186306e6632f94df0275d5e172210ae3044b54
Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

HID: sensor-hub: Add sensor_hub_input_attr_read_values() for multi-byte reads

This commit introduces sensor_hub_input_attr_read_values() to allow
multi-byte reads for sensors reporting data larger than 32 bits. It
uses a caller-provided buffer to accumulate incoming data in
sensor_hub_raw_event() until the buffer is full.

> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c

[ ... ]

> +	if (flag == SENSOR_HUB_SYNC) {
> +		cycles = wait_for_completion_interruptible_timeout(&hsdev->pending.ready,
> +								   HZ * 5);
> +		if (cycles == 0)
> +			ret = -ETIMEDOUT;
> +		else if (cycles < 0)
> +			ret = cycles;
> +
> +		hsdev->pending.status = false;

[Severity: Critical]
Does setting this status flag without acquiring the data->lock introduce a
race condition?

If wait_for_completion_interruptible_timeout() times out or is interrupted,
this locklessly sets hsdev->pending.status = false and returns. Concurrently,
the interrupt handler sensor_hub_raw_event() might have already observed
status as true and could be mid-copy into the caller-provided buffer.

Returning early would invalidate the caller-provided buffer, and the
interrupt handler might subsequently overwrite the destroyed stack frame.

> +	}
> +	mutex_unlock(hsdev->mutex_ptr);
> +
> +	return ret;
> +}

[ ... ]

> @@ -518,12 +568,27 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
>  					      hsdev->pending.attr_usage_id ==
>  					      report->field[i]->logical)) {
>  			hid_dbg(hdev, "data was pending ...\n");
> -			hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
> -			if (hsdev->pending.raw_data)
> -				hsdev->pending.raw_size = sz;
> -			else
> -				hsdev->pending.raw_size = 0;
> -			complete(&hsdev->pending.ready);
> +			if (hsdev->pending.max_raw_size) {
> +				if (hsdev->pending.index < hsdev->pending.max_raw_size) {
> +					avail = hsdev->pending.max_raw_size - hsdev->pending.index;
> +					copy_size = clamp(sz, 0U, avail);
> +
> +					memcpy(hsdev->pending.raw_data + hsdev->pending.index,
> +					       ptr, copy_size);

[Severity: Critical]
As mentioned above, if sensor_hub_input_attr_read_values() aborts early,
could this memcpy() write into an invalidated stack buffer?

> +					hsdev->pending.index += copy_size;
> +					if (hsdev->pending.index >= hsdev->pending.max_raw_size) {
> +						hsdev->pending.raw_size = hsdev->pending.index;
> +						complete(&hsdev->pending.ready);
> +					}
> +				}
> +			} else {
> +				hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);

[Severity: High]
This is a pre-existing issue, but does this allocation leak memory if a
crafted HID report contains multiple duplicate fields matching the
requested usage?

If the device rapidly spams events while a reader is waiting, each match
could trigger a new kmemdup, overwriting the hsdev->pending.raw_data
pointer and permanently leaking the previously allocated blocks.

> +				if (hsdev->pending.raw_data)
> +					hsdev->pending.raw_size = sz;
> +				else
> +					hsdev->pending.raw_size = 0;
> +				complete(&hsdev->pending.ready);
> +			}
>  		}
>  		if (callback->capture_sample) {
>  			if (report->field[i]->logical)

[Severity: High]
This is a pre-existing issue, but does the loop advancing ptr expose
uninitialized heap memory on short reports?

Looking at the loop in sensor_hub_raw_event():

	for (i = 0; i < report->maxfield; ++i) {
		...
		sz = (report->field[i]->report_size *
					report->field[i]->report_count)/8;
		...
		ptr += sz;
	}

The pointer is unconditionally advanced by the expected sizes defined in
the report descriptor. Since sensor_hub_raw_event() ignores the actual
size argument representing the received packet length, a maliciously
short packet could cause the driver to read past the initialized packet
bounds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610082911.157232-1-lixu.zhang@intel.com?part=1