[Bug 221077] [iio] [hid-sensor-rotation] Memory corruption due to alignment mismatch in scan buffer

[bugzilla-daemon@kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=221077

--- Comment #2 from dlechner@baylibre.com ---
On 2/14/26 1:24 PM, Jonathan Cameron wrote:
> On Wed, 11 Feb 2026 05:12:36 +0000
> bugzilla-daemon@kernel.org wrote:
> 
>> https://bugzilla.kernel.org/show_bug.cgi?id=221077
>>
>>             Bug ID: 221077
>>            Summary: [iio] [hid-sensor-rotation] Memory corruption due to
>>                     alignment mismatch in scan buffer
>>            Product: Drivers
>>            Version: 2.5
>>           Hardware: All
>>                 OS: Linux
>>             Status: NEW
>>           Severity: normal
>>           Priority: P3
>>          Component: IIO
>>           Assignee: drivers_iio@kernel-bugs.kernel.org
>>           Reporter: lixu.zhang@intel.com
>>         Regression: Yes
>>            Bisected b31a74075cb4ca2bb202a2e17d133ef3c9ee891f
>>          commit-id:
>>
>> ### Problem
>> In `drivers/iio/orientation/hid-sensor-rotation.c`, the `scale_pre_decml`
>> and
>> `scale_post_decml` fields in `struct dev_rot_state` get corrupted after the
>> first read from the device. This issue results in invalid scale values being
>> reported to userspace.
>>
>> ### Root Cause Analysis
>> The issue is caused by a size mismatch between what the IIO core expects for
>> the scan buffer and the actual size of the driver's scan structure.
>>
>> 1.  **Driver Structure**: The `scan` struct in `dev_rot_state` consists of a
>> quaternion (4 * s32 = 16 bytes) and a timestamp (8 bytes).
>>     ```c
>>     struct {
>>             s32 sampled_vals[4];
>>             aligned_s64 timestamp;
>>     } scan;
>>     ```
>>     Without explicit alignment, this structure is packed to **24 bytes**.
> 
> I agree it's 24 bytes, but worth being clear it has explicit alignment to 8
> bytes. Without that on some platforms it would be 4 byte aligned only.
> 
>>
>> 2.  **IIO Core Expectation**: The `iio_compute_scan_bytes` function
>> calculates
>> the buffer size required. It aligns the total size to the size of the
>> *largest
>> element* in the scan.
>>     - The quaternion channel is treated as a single 16-byte element.
>>     - Therefore, the core aligns the total size to 16 bytes: `ALIGN(24, 16)
>>     =
>> 32 bytes`.
> 
> Ah. That is indeed a corner case and the ABI is IIRC not well documented
> around that, let alone what we do in the kernel. We probably need to tighten
> that
> up as well as fixing this. What userspace were you using to test this?

I didn't even know we had IIO_MOD_QUATERNION as a special data type until
Francesco's recent patches. :-o

> 
> 
>>
>> 3.  **Corruption**: When `iio_push_to_buffers_with_timestamp()` is called:
>>     - It assumes a 32-byte buffer.
>>     - It writes the timestamp at the end of the aligned buffer (offset 24).
>>     - Since the driver allocated only 24 bytes for `scan`, the write at
>>     offset
>> 24 overwrites the adjacent `scale_pre_decml` field in `struct
>> dev_rot_state`.
>>
>> ### Evidence (Ftrace)
>> I verified this by tracing the return values of `iio_storage_bytes_for_si`
>> and
>> `iio_compute_scan_bytes` using kretprobes:
>>
>> ```log
>> $ cat /sys/kernel/tracing/trace_pipe
>> r_store_bytes: (iio_compute_scan_bytes+0x30/0xd0 [industrialio] <-
>> iio_storage_bytes_for_si) arg1=0x10
>> r_store_bytes: (iio_compute_scan_bytes+0xa1/0xd0 [industrialio] <-
>> iio_storage_bytes_for_si) arg1=0x8
>> r_calc_bytes: (__iio_update_buffers+0x99d/0xd40 [industrialio] <-
>> iio_compute_scan_bytes) arg1=0x20
>> ```
>>
>> The trace confirms:
>> - Largest element = 16 bytes.
>> - Total raw size = 16 (quat) + 8 (ts) = 24 bytes.
>> - Final aligned size = ALIGN(24, 16) = 32 bytes.
>>
>> The memory layout mismatch is:
>> - IIO Core needs: 32 bytes
>> - Driver struct has: 24 bytes
>>
>> ### Regression
>> This issue was introduced by commit `b31a74075cb4 ("iio: orientation:
>> hid-sensor-rotation: remove unnecessary alignment")`, which removed the
>> `__aligned(16)` attribute that previously ensured the struct was padded to
>> 32
>> bytes.
>>
>> ### Proposed Fix
>> Revert the removal of `__aligned(16)` to ensure `struct dev_rot_state` has
>> the
>> correct padding to match the IIO core's expectations.
>>
>> ```c
>> struct {
>>         s32 sampled_vals[4] __aligned(16);
> Agreed that fix is correct.
> However, we definitely also need to add some documentation on why it is
> there.
> 
> It's not actually an alignment force on sampled_vals, but rather on the
> timestamp.
> We can't just mark the timestamp as then it will have two aligned markings on
> x86_32.
> 
> David, given we both missed this when 'tidying' this up wrongly what do you
> think
> would make this clearest?

I think it would make sense to have an IIO_DECLARE_REPEAT_ELEMENT()
similar to IIO_DECLARE_BUFFER_WITH_TS().

#define IIO_DECLARE_REPEAT_ELEMENT(type, name, count) \
        type name[count] __aligned(sizeof(type) * count)

And it would be used like:

struct {
        IIO_DECLARE_REPEAT_ELEMENT(s32, sampled_vals, 4);
        aligned_s64 timestamp;
} scan;

> 
> 
> Thanks for reporting this with such a thorough and detailed investigation!
> Makes our lives a lot easier.

Should I spin up a fix?

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.