On Wed, 2020-07-22 at 16:50 +0100, Jonathan Cameron 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.
> 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 structure in the iio_priv() data with alignment
> ensured by use of an explicit c structure. This data is allocated
> with kzalloc so no data can leak appart from previous readings.
>
> Fixes tag is beyond some major refactoring so likely manual
> backporting
> would be needed to get that far back.
>
> Whilst the force alignment of the ts is not strictly necessary, it
> does make the code less fragile.
>
> Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware
> fifo")
> Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
> drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/bmc150-accel-core.c
> b/drivers/iio/accel/bmc150-accel-core.c
> index 24864d9dfab5..48435865fdaf 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -189,6 +189,14 @@ struct bmc150_accel_data {
> struct mutex mutex;
> u8 fifo_mode, watermark;
> s16 buffer[8];
> + /*
> + * Ensure there is sufficient space and correct alignment for
> + * the timestamp if enabled
> + */
> + struct {
> + __le16 channels[3];
> + s64 ts __aligned(8);
> + } scan;
> u8 bw_bits;
> u32 slope_dur;
> u32 slope_thres;
> @@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct
> iio_dev *indio_dev,
> * now.
> */
> for (i = 0; i < count; i++) {
> - u16 sample[8];
> int j, bit;
>
> j = 0;
> for_each_set_bit(bit, indio_dev->active_scan_mask,
> indio_dev->masklength)
> - memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
> + memcpy(&data->scan.channels[j++], &buffer[i * 3
> + bit],
> + sizeof(data->scan.channels[0]));
>
> - iio_push_to_buffers_with_timestamp(indio_dev, sample,
> tstamp);
> + iio_push_to_buffers_with_timestamp(indio_dev, &data-
> >scan,
> + tstamp);
>
> tstamp += sample_period;
> }
