[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 #3 from Jonathan Cameron (jic23@kernel.org) ---
On Sat, 14 Feb 2026 14:29:19 -0600
David Lechner <dlechner@baylibre.com> wrote:

> 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

It's not common.  Was interestingly something that looked likely to be so
in relatively early IMUs but then faded away -> probably because the algorithms
to get a good estimate are complex and best done on the host CPU or somewhere
off sensor.

> 
> > 
> >   
> >>
> >> 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().

It will be worth checking if we have more than one user.  When quaternion's
turn up there are always other channels.  The oddity is that sometimes the
fusion
algorithms used produce the data at a different rate to the underlying channel
types (or like in HID's case) don't tell you what that relationship is, data
just
shows up tagged with what type it is and we split it to the different IIO
devices.

As such they at least sometimes share a buffer with non repeated channels.
> 
> #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;

Isn't our timestamp ending up in the wrong place anyway vs that
definition (and the original one pre bug)? There is a lazy hack in
iio_push_to_buffers_with_ts() that assumes the structure is 8 byte aligned :(
and puts the timestamp in the last 8 bytes but per normal channel packing each
element is naturally aligned so it should be buffer + 16, not buffer + 32 - 8

We still need the extra padding the aligned is bringing because the kfifo when
the configuration matches this layout has 32 byte entries, but the timestamp
should be where it this structure says it is not where it's currently going.

Given how common that iio_push_to_buffers_with_timestamp()/ts() is
(and fiddly computing the timestamp position is) I'd suggest in this case we
open code iio_push_to_buffers_with_ts().  We can swing by after this bug is
fixed and try to work out if there is a cleaner solution (probably compute as
part of the demux setup and stash it for reuse).

+ Change to _ts() version as it would have screamed about this without the
corruption see by the reporter as the buffer would have been smaller than the
kfifo was expecting.

This is a userspace ABI bug fix / change. So we need to know what userspace is
making
use of quaternion channels and fix it as well.

Jonathan


> } scan;
> 
> > 
> > 
> > Thanks for reporting this with such a thorough and detailed investigation!
> > Makes our lives a lot easier.  
> 
> Should I spin up a fix?
> 
If you have time to take on also landing the timestamp in the right place then
that would be great. I'm a touch snowed under for the next week or two.

Jonathan

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

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