iio: gyro: itg3200: fix i2c read into the wrong stack location

itg3200_read_all_channels() takes __be16 *buf' as a parameter and fills the i2c_msg destination as (char *)&buf'. Since buf' is the parameter (a pointer), &buf' is the address of the local pointer slot on the stack of itg3200_read_all_channels(), not the address of the caller's scan buffer. The (char *) cast hides the type mismatch.

i2c_transfer() therefore writes ITG3200_SCAN_ELEMENTS * sizeof(s16) = 8 bytes into the parameter's stack slot, which is discarded when the function returns. The caller's scan buffer in itg3200_trigger_handler() is never written to, so iio_push_to_buffers_with_timestamp() pushes uninitialised stack contents to userspace via /dev/iio:deviceX every scan -- both a functional bug (no actual gyroscope or temperature data is delivered through the triggered buffer) and an information leak.

The non-buffered read_raw() path is unaffected: it goes through itg3200_read_reg_s16() which uses `&out' on a local s16 value, where that is correct.

Drop the spurious `&' so the i2c read writes into the caller's buffer.

Quoted 16 lines · David Carlier

itg3200_read_all_channels() takes __be16 *buf' as a parameter and fills the i2c_msg destination as (char *)&buf'. Since buf' is the parameter (a pointer), &buf' is the address of the local pointer slot on the stack of itg3200_read_all_channels(), not the address of the caller's scan buffer. The (char *) cast hides the type mismatch.

i2c_transfer() therefore writes ITG3200_SCAN_ELEMENTS * sizeof(s16) = 8 bytes into the parameter's stack slot, which is discarded when the function returns. The caller's scan buffer in itg3200_trigger_handler() is never written to, so iio_push_to_buffers_with_timestamp() pushes uninitialised stack contents to userspace via /dev/iio:deviceX every scan -- both a functional bug (no actual gyroscope or temperature data is delivered through the triggered buffer) and an information leak.

Quoted 6 lines

The non-buffered read_raw() path is unaffected: it goes through itg3200_read_reg_s16() which uses `&out' on a local s16 value, where that is correct.

Drop the spurious `&' so the i2c read writes into the caller's buffer.

Very good catch! I'm puzzled if that code was ever tested. Do you have an HW and that's how you enter to this bug?

Reviewed-by Andy Shevchenko <[email protected]>

--
With Best Regards,
Andy Shevchenko

Quoted 25 lines · Andy Shevchenko

Quoted 16 lines · David Carlier

itg3200_read_all_channels() takes __be16 *buf' as a parameter and fills the i2c_msg destination as (char *)&buf'. Since buf' is the parameter (a pointer), &buf' is the address of the local pointer slot on the stack of itg3200_read_all_channels(), not the address of the caller's scan buffer. The (char *) cast hides the type mismatch.

i2c_transfer() therefore writes ITG3200_SCAN_ELEMENTS * sizeof(s16) = 8 bytes into the parameter's stack slot, which is discarded when the function returns. The caller's scan buffer in itg3200_trigger_handler() is never written to, so iio_push_to_buffers_with_timestamp() pushes uninitialised stack contents to userspace via /dev/iio:deviceX every scan -- both a functional bug (no actual gyroscope or temperature data is delivered through the triggered buffer) and an information leak.

Quoted 6 lines

The non-buffered read_raw() path is unaffected: it goes through itg3200_read_reg_s16() which uses `&out' on a local s16 value, where that is correct.

Drop the spurious `&' so the i2c read writes into the caller's buffer.

Quoted 10 lines

Very good catch! I'm puzzled if that code was ever tested. Do you have an HW and that's how you enter to this bug?

Reviewed-by Andy Shevchenko <[email protected]>

--
With Best Regards,
Andy Shevchenko

Thanks! No HW on my side -- found by inspection. I had recently looked
at a similar (char *)&buf' / (char *)buf' mix-up in another
driver,
so I went grepping for the same shape and itg3200 stood out. For contrast, drivers/iio/humidity/hdc3020.c::hdc3020_read_bytes() has
the
same signature (u8 *buf parameter) and assigns `.buf = buf'
correctly.

Compile-tested only; the analysis in the changelog is what I'm
relying
on.

Cheers !