Re: [PATCH 6/7] iio: orientation: hid-sensor-rotation: use u32 instead of unsigned

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds read and information leak due to unverified raw_len in dev_rot_capture_sample().
- [High] Unaligned and endian-unsafe access when parsing HID raw byte streams.
--

commit 1678aae0ba9cfb6e8824b0811f3f770a159c5a16
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: orientation: hid-sensor-rotation: use u32 instead of unsigned

This commit replaces bare unsigned types with u32 for usage_id parameters
across the HID sensor rotation driver to improve code clarity and
consistency with kernel style.

> --- a/drivers/iio/orientation/hid-sensor-rotation.c
> +++ b/drivers/iio/orientation/hid-sensor-rotation.c

[ ... ]

> @@ -209,7 +209,7 @@ static int dev_rot_proc_event(struct hid_sensor_hub_device *hsdev,
>  
>  /* Capture samples in local storage */
>  static int dev_rot_capture_sample(struct hid_sensor_hub_device *hsdev,
> -				unsigned usage_id,
> +				u32 usage_id,
>  				size_t raw_len, char *raw_data,
>  				void *priv)
>  {

[Severity: High]
This is a pre-existing issue, but does this function perform unaligned and
endian-unsafe reads?

Looking further down in dev_rot_capture_sample(), when raw_data is accessed:

        if (raw_len / 4 == sizeof(s16)) {
            rot_state->scan.sampled_vals[0] = ((s16 *)raw_data)[0];
            rot_state->scan.sampled_vals[1] = ((s16 *)raw_data)[1];
            rot_state->scan.sampled_vals[2] = ((s16 *)raw_data)[2];
            rot_state->scan.sampled_vals[3] = ((s16 *)raw_data)[3];

Since raw_data points to a byte array from a tightly packed HID report, it
is not guaranteed to be aligned to a 2-byte boundary. Casting directly to
s16 pointers might cause bus errors or panics on architectures that do not
support unaligned loads. 

Also, native CPU casts will read garbage values on big-endian architectures. 
Should this use get_unaligned_le16() instead?

[Severity: High]
This isn't a bug introduced by this patch, but can the fallback memcpy 
result in an out-of-bounds read?

If raw_len is not exactly 8 bytes, the code unconditionally copies 16 bytes:

        } else {
            memcpy(&rot_state->scan.sampled_vals, raw_data,
                   sizeof(rot_state->scan.sampled_vals));
        }

If a malformed or malicious HID descriptor specifies a size smaller than
16 bytes, this would read past the end of the raw_data buffer into adjacent
kernel memory. 

The copied data could then be pushed to userspace via iio_push_to_buffers(),
potentially creating an information leak.

[Severity: High]
This is a pre-existing issue, but does the timestamp handling also have
bounds checking and alignment issues?

Later in the same function:

    } else if (usage_id == HID_USAGE_SENSOR_TIME_TIMESTAMP) {
        rot_state->timestamp = hid_sensor_convert_timestamp(
                                    &rot_state->common_attributes,
                                    *(s64 *)raw_data);

The raw_data pointer is cast directly to a 64-bit integer without verifying
that raw_len is at least 8 bytes. 

Similar to the unaligned reads above, a direct cast to an s64 pointer on an
unaligned buffer could cause bus errors on certain architectures, and it is
also not endian-safe.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-6-june-hid-iio-correct-usage-id-v1-0-dd4a6820b674@gmail.com?part=6