Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, May 25, 2020 at 06:06:22PM +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 an array of smaller elements on the stack.
> 
> Here we use an explicit structure and rely on that to enforce
> alignment on the stack.  Note there was never a data leak here
> due to the explicit memset.
> 
> Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/adc/ti-ads1015.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 5ea4f45d6bad..05853723dbdb 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ads1015_data *data = iio_priv(indio_dev);
> -	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
> +	/* Ensure natural alignment for buffer elements */
> +	struct {
> +		s16 channel;
> +		s64 ts;
> +	} scan;

Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
iio_push_to_buffers_with_timestamp() point of view?

>  	int chan, ret, res;
>  
> -	memset(buf, 0, sizeof(buf));
> +	memset(&scan, 0, sizeof(scan));
>  
>  	mutex_lock(&data->lock);
>  	chan = find_first_bit(indio_dev->active_scan_mask,
> @@ -399,10 +403,10 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>  		goto err;
>  	}
>  
> -	buf[0] = res;
> +	scan.channel = res;
>  	mutex_unlock(&data->lock);
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
>  					   iio_get_time_ns(indio_dev));
>  
>  err:
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko