Re: [PATCH v3 03/27] iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 29 Jul 2020 10:12:36 -0700
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> On Wed, 2020-07-22 at 16:50 +0100, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > 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@metafoo.de>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
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;
> >  	}  
>