On Wed, 22 Jul 2020 16:50:41 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes). This is not guaranteed in
> this driver which uses a 16 byte array of smaller elements on the stack.
> This is fixed by using an explicit c structure. As there are no
> holes in the structure, there is no possiblity of data leakage
> in this case.
>
> The explicit alignment of ts is not strictly necessary but potentially
> makes the code slightly less fragile. It also removes the possibility
> of this being cut and paste into another driver where the alignment
> isn't already true.
>
> Fixes: 36e0371e7764 ("iio:itg3200: Use iio_push_to_buffers_with_timestamp()")
> Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
No idea why I didn't pick this one up with the other similar cases as
it fits in Andy's case 1 (no change needed).
Anyhow, now applied to the togreg branch of iio.git as no particular
rush to get this in.
Cc'd stable.
> ---
> drivers/iio/gyro/itg3200_buffer.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/gyro/itg3200_buffer.c b/drivers/iio/gyro/itg3200_buffer.c
> index d3fbe9d86467..1c3c1bd53374 100644
> --- a/drivers/iio/gyro/itg3200_buffer.c
> +++ b/drivers/iio/gyro/itg3200_buffer.c
> @@ -46,13 +46,20 @@ static irqreturn_t itg3200_trigger_handler(int irq, void *p)
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct itg3200 *st = iio_priv(indio_dev);
> - __be16 buf[ITG3200_SCAN_ELEMENTS + sizeof(s64)/sizeof(u16)];
> -
> - int ret = itg3200_read_all_channels(st->i2c, buf);
> + /*
> + * Ensure correct alignment and padding including for the
> + * timestamp that may be inserted.
> + */
> + struct {
> + __be16 buf[ITG3200_SCAN_ELEMENTS];
> + s64 ts __aligned(8);
> + } scan;
> +
> + int ret = itg3200_read_all_channels(st->i2c, scan.buf);
> if (ret < 0)
> goto error_ret;
>
> - iio_push_to_buffers_with_timestamp(indio_dev, buf, pf->timestamp);
> + iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
>
> iio_trigger_notify_done(indio_dev->trig);
>
