[v3,02/27] iio:accel:mma8452: Fix timestamp alignment and prevent data leak.
diff mbox series

Message ID 20200722155103.979802-3-jic23@kernel.org
State New
Headers show
Series
  • IIO: Fused set 1 and 2 of timestamp alignment fixes
Related show

Commit Message

Jonathan Cameron July 22, 2020, 3:50 p.m. UTC
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 a 16 byte u8 array 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 with alignment
ensured by use of an explicit c structure.  This data is allocated
with kzalloc so no data can leak appart from previous readings.

The additional forcing of the 8 byte alignment of the timestamp
is not strictly necessary but makes the code less fragile by
making this explicit.

Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma8452.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron Aug. 9, 2020, 5:14 p.m. UTC | #1
On Wed, 22 Jul 2020 16:50:38 +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 a 16 byte u8 array 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 with alignment
> ensured by use of an explicit c structure.  This data is allocated
> with kzalloc so no data can leak appart from previous readings.
> 
> The additional forcing of the 8 byte alignment of the timestamp
> is not strictly necessary but makes the code less fragile by
> making this explicit.
> 
> Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/mma8452.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index ba27f8673131..1cf2b5db26ca 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -110,6 +110,12 @@ struct mma8452_data {
>  	int sleep_val;
>  	struct regulator *vdd_reg;
>  	struct regulator *vddio_reg;
> +
> +	/* Ensure correct alignment of time stamp when present */
> +	struct {
> +		__be16 channels[3];
> +		s64 ts __aligned(8);
> +	} buffer;
>  };
>  
>   /**
> @@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>  	int ret;
>  
> -	ret = mma8452_read(data, (__be16 *)buffer);
> +	ret = mma8452_read(data, data->buffer.channels);
>  	if (ret < 0)
>  		goto done;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
>  					   iio_get_time_ns(indio_dev));
>  
>  done:

Patch
diff mbox series

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index ba27f8673131..1cf2b5db26ca 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -110,6 +110,12 @@  struct mma8452_data {
 	int sleep_val;
 	struct regulator *vdd_reg;
 	struct regulator *vddio_reg;
+
+	/* Ensure correct alignment of time stamp when present */
+	struct {
+		__be16 channels[3];
+		s64 ts __aligned(8);
+	} buffer;
 };
 
  /**
@@ -1091,14 +1097,13 @@  static irqreturn_t mma8452_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mma8452_data *data = iio_priv(indio_dev);
-	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
 	int ret;
 
-	ret = mma8452_read(data, (__be16 *)buffer);
+	ret = mma8452_read(data, data->buffer.channels);
 	if (ret < 0)
 		goto done;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
 					   iio_get_time_ns(indio_dev));
 
 done: