On Wed, 29 Jul 2020 10:12:36 -0700
Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> 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>
Applied to the fixes-togreg branch of iio.git and marked for stable.
Thanks,
Jonathan
>
> > ---
> > 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;
> > }
>
