Re: [PATCH 13/32] iio:magnetometer:ak8974: Fix alignment and data leak issues

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  7 Jun 2020 16:53:49 +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() data.
> 
> This data is allocated with kzalloc so no data can leak appart from
> previous readings.
> 
> Fixes: 7c94a8b2ee8cf ("iio: magn: add a driver for AK8974")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied to the fixes-togreg branch of iio.git. I'm picking up
all the ones in the series which I have had positive feedback on.

Thanks,

Jonathan

> ---
>  drivers/iio/magnetometer/ak8974.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> index c2260c84f7f1..ea09b549ec4e 100644
> --- a/drivers/iio/magnetometer/ak8974.c
> +++ b/drivers/iio/magnetometer/ak8974.c
> @@ -192,6 +192,11 @@ struct ak8974 {
>  	bool drdy_irq;
>  	struct completion drdy_complete;
>  	bool drdy_active_low;
> +	/* Ensure timestamp is naturally aligned */
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
>  };
>  
>  static const char ak8974_reg_avdd[] = "avdd";
> @@ -657,7 +662,6 @@ static void ak8974_fill_buffer(struct iio_dev *indio_dev)
>  {
>  	struct ak8974 *ak8974 = iio_priv(indio_dev);
>  	int ret;
> -	__le16 hw_values[8]; /* Three axes + 64bit padding */
>  
>  	pm_runtime_get_sync(&ak8974->i2c->dev);
>  	mutex_lock(&ak8974->lock);
> @@ -667,13 +671,13 @@ static void ak8974_fill_buffer(struct iio_dev *indio_dev)
>  		dev_err(&ak8974->i2c->dev, "error triggering measure\n");
>  		goto out_unlock;
>  	}
> -	ret = ak8974_getresult(ak8974, hw_values);
> +	ret = ak8974_getresult(ak8974, ak8974->scan.channels);
>  	if (ret) {
>  		dev_err(&ak8974->i2c->dev, "error getting measures\n");
>  		goto out_unlock;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, hw_values,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &ak8974->scan,
>  					   iio_get_time_ns(indio_dev));
>  
>   out_unlock: