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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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>

> ---
>  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;
>  	}