Re: [PATCH v3 12/27] iio:light:ltr501 Fix timestamp alignment issue.

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 22 Jul 2020 16:50:48 +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.
> Here we use a structure on the stack.  The driver already did an
> explicit memset so no data leak was possible.
> 
> Forced alignment of ts is not strictly necessary but probably makes
> the code slightly less fragile.
> 
> Note there has been some rework in this driver of the years, so no
> way this will apply cleanly all the way back.
> 
> Fixes: 2690be905123 ("iio: Add Lite-On ltr501 ambient light / proximity sensor driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/light/ltr501.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 4bac0646398d..b4323d2db0b1 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c
> @@ -1243,13 +1243,16 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ltr501_data *data = iio_priv(indio_dev);
> -	u16 buf[8];
> +	struct {
> +		u16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
>  	__le16 als_buf[2];
>  	u8 mask = 0;
>  	int j = 0;
>  	int ret, psdata;
>  
> -	memset(buf, 0, sizeof(buf));
> +	memset(&scan, 0, sizeof(scan));
>  
>  	/* figure out which data needs to be ready */
>  	if (test_bit(0, indio_dev->active_scan_mask) ||
> @@ -1268,9 +1271,9 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
>  		if (ret < 0)
>  			return ret;
>  		if (test_bit(0, indio_dev->active_scan_mask))
> -			buf[j++] = le16_to_cpu(als_buf[1]);
> +			scan.channels[j++] = le16_to_cpu(als_buf[1]);
>  		if (test_bit(1, indio_dev->active_scan_mask))
> -			buf[j++] = le16_to_cpu(als_buf[0]);
> +			scan.channels[j++] = le16_to_cpu(als_buf[0]);
>  	}
>  
>  	if (mask & LTR501_STATUS_PS_RDY) {
> @@ -1278,10 +1281,10 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
>  				       &psdata, 2);
>  		if (ret < 0)
>  			goto done;
> -		buf[j++] = psdata & LTR501_PS_DATA_MASK;
> +		scan.channels[j++] = psdata & LTR501_PS_DATA_MASK;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
>  					   iio_get_time_ns(indio_dev));
>  
>  done: