On Wed, 22 Jul 2020 16:51:01 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> 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 an array of smaller elements on the stack.
>
> We move to a suitable structure in the iio_priv() data with alignment
> explicitly requested. This data is allocated with kzalloc so no
> data can leak apart from previous readings. Note that previously
> no leak at all could occur, but previous readings should never
> be a problem.
>
> In this case the timestamp location depends on what other channels
> are enabled. As such we can't use a structure without misleading
> by suggesting only one possible timestamp location.
>
> Fixes: 50a6edb1b6e0 ("iio: adc: add ADC12130/ADC12132/ADC12138 ADC driver")
> Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Applied with expand comment.
Thanks,
Jonathan
> ---
> drivers/iio/adc/ti-adc12138.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-adc12138.c b/drivers/iio/adc/ti-adc12138.c
> index e485719cd2c4..16f4fd7a04d9 100644
> --- a/drivers/iio/adc/ti-adc12138.c
> +++ b/drivers/iio/adc/ti-adc12138.c
> @@ -47,6 +47,8 @@ struct adc12138 {
> struct completion complete;
> /* The number of cclk periods for the S/H's acquisition time */
> unsigned int acquisition_time;
> + /* 16x 2 bytes ADC data + 8 bytes timestamp */
> + __be16 data[20] __aligned(8);
>
> u8 tx_buf[2] ____cacheline_aligned;
> u8 rx_buf[2];
> @@ -329,7 +331,6 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct adc12138 *adc = iio_priv(indio_dev);
> - __be16 data[20] = { }; /* 16x 2 bytes ADC data + 8 bytes timestamp */
> __be16 trash;
> int ret;
> int scan_index;
> @@ -345,7 +346,7 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
> reinit_completion(&adc->complete);
>
> ret = adc12138_start_and_read_conv(adc, scan_chan,
> - i ? &data[i - 1] : &trash);
> + i ? &adc->data[i - 1] : &trash);
> if (ret) {
> dev_warn(&adc->spi->dev,
> "failed to start conversion\n");
> @@ -362,7 +363,7 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
> }
>
> if (i) {
> - ret = adc12138_read_conv_data(adc, &data[i - 1]);
> + ret = adc12138_read_conv_data(adc, &adc->data[i - 1]);
> if (ret) {
> dev_warn(&adc->spi->dev,
> "failed to get conversion data\n");
> @@ -370,7 +371,7 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
> }
> }
>
> - iio_push_to_buffers_with_timestamp(indio_dev, data,
> + iio_push_to_buffers_with_timestamp(indio_dev, adc->data,
> iio_get_time_ns(indio_dev));
> out:
> mutex_unlock(&adc->lock);
