Re: [PATCH v3 21/27] iio:adc:ti-adc084s021 Fix alignment and data leak issues.

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 22 Jul 2020 16:50:57 +0100
Jonathan Cameron <jic23@kernel.org> 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.
> 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().
> 
> This data is allocated with kzalloc so no data can leak apart from
> previous readings.
> 
> The force alignment of ts is not strictly necessary in this case
> but reduces the fragility of the code.
> 
> Fixes: 3691e5a69449 ("iio: adc: add driver for the ti-adc084s021 chip")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Mårten Lindahl <martenli@axis.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable.

thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-adc084s021.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
> index 9017e1e24273..dfba34834a57 100644
> --- a/drivers/iio/adc/ti-adc084s021.c
> +++ b/drivers/iio/adc/ti-adc084s021.c
> @@ -26,6 +26,11 @@ struct adc084s021 {
>  	struct spi_transfer spi_trans;
>  	struct regulator *reg;
>  	struct mutex lock;
> +	/* Buffer used to align data */
> +	struct {
> +		__be16 channels[4];
> +		s64 ts __aligned(8);
> +	} scan;
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache line.
> @@ -141,14 +146,13 @@ static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void *pollfunc)
>  	struct iio_poll_func *pf = pollfunc;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct adc084s021 *adc = iio_priv(indio_dev);
> -	__be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes timestamp */
>  
>  	mutex_lock(&adc->lock);
>  
> -	if (adc084s021_adc_conversion(adc, &data) < 0)
> +	if (adc084s021_adc_conversion(adc, adc->scan.channels) < 0)
>  		dev_err(&adc->spi->dev, "Failed to read data\n");
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, data,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
>  					   iio_get_time_ns(indio_dev));
>  	mutex_unlock(&adc->lock);
>  	iio_trigger_notify_done(indio_dev->trig);