On Wed, 22 Jul 2020 16:50:51 +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 an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here. We close both issues by
> moving to a suitable array in the iio_priv() data with alignment
> explicitly requested. This data is allocated with kzalloc so no
> data can leak apart from previous readings.
>
> In this driver, depending on which channels are enabled, the timestamp
> can be in a number of locations. Hence we cannot use a structure
> to specify the datalayout without it being missleading.
>
> Fixes: 77c4ad2d6a9b ("iio: imu: Add initial support for Bosch BMI160")
> Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Cc: Daniel Baluta <daniel.baluta@xxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
I applied this one then backed it out after realising that the device doesn't
have 9 channels despite the comment in place saying it does.
This particular device only has an accelerometer and a gyroscope, each
with 3 axes.
Daniel, can you confirm my interpretation on that?
I think we only need a buffer with space for
6 __le16 channels, 4 bytes padding and an s64 for the
timestamp.
So __le16 buffer[12].
Thanks,
Jonathan
> ---
> drivers/iio/imu/bmi160/bmi160.h | 2 ++
> drivers/iio/imu/bmi160/bmi160_core.c | 5 ++---
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index a82e040bd109..d29f1b5d1658 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -10,6 +10,8 @@ struct bmi160_data {
> struct iio_trigger *trig;
> struct regulator_bulk_data supplies[2];
> struct iio_mount_matrix orientation;
> + /* Ensure natural alignment for timestamp if present */
> + __le16 buf[16] __aligned(8);
> };
>
> extern const struct regmap_config bmi160_regmap_config;
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 222ebb26f013..86cfd75ea125 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -427,7 +427,6 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct bmi160_data *data = iio_priv(indio_dev);
> - __le16 buf[16];
> /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */
> int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
> __le16 sample;
> @@ -438,10 +437,10 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> &sample, sizeof(sample));
> if (ret)
> goto done;
> - buf[j++] = sample;
> + data->buf[j++] = sample;
> }
>
> - iio_push_to_buffers_with_timestamp(indio_dev, buf, pf->timestamp);
> + iio_push_to_buffers_with_timestamp(indio_dev, data->buf, pf->timestamp);
> done:
> iio_trigger_notify_done(indio_dev->trig);
> return IRQ_HANDLED;
