[12/25] iio:imu:st_lsm6dsx Fix alignment and data leak issues
diff mbox series

Message ID 20200525170628.503283-13-jic23@kernel.org
State New
Headers show
Series
  • IIO: 2nd set of timestamp alignment fixes.
Related show

Commit Message

Jonathan Cameron May 25, 2020, 5:06 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 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 set of suitable structures in the iio_priv() data.

This data is allocated with kzalloc so no data can leak apart from
previous readings.

There has been a lot of churn in this driver, so likely backports
may be needed for stable.

Fixes: 290a6ce11d93 ("iio: imu: add support to lsm6dsx driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  5 ++++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 27 ++++++++++---------
 2 files changed, 20 insertions(+), 12 deletions(-)

Comments

Lorenzo Bianconi May 26, 2020, 7:58 a.m. UTC | #1
> 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 set of suitable structures in the iio_priv() data.
> 
> This data is allocated with kzalloc so no data can leak apart from
> previous readings.
> 
> There has been a lot of churn in this driver, so likely backports
> may be needed for stable.
> 
> Fixes: 290a6ce11d93 ("iio: imu: add support to lsm6dsx driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  5 ++++
>  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 27 ++++++++++---------
>  2 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index b56df409ed0f..5bc724eadc83 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -411,6 +411,11 @@ struct st_lsm6dsx_hw {
>  	const struct st_lsm6dsx_settings *settings;
>  
>  	struct iio_mount_matrix orientation;
> +	/* Ensure natural alignment of buffer elements */
> +	struct {
> +		__le16 channels[3];
> +		s64 ts;
> +	} gyro_scan, acc_scan, ext_scan;
>  };
>  
>  static __maybe_unused const struct iio_event_spec st_lsm6dsx_event = {
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index afd00daeefb2..9bcffbfac797 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -341,9 +341,6 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>  	int err, sip, acc_sip, gyro_sip, ts_sip, ext_sip, read_len, offset;
>  	u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
>  	u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
> -	u8 gyro_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
> -	u8 acc_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
> -	u8 ext_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
>  	bool reset_ts = false;
>  	__le16 fifo_status;
>  	s64 ts = 0;
> @@ -404,18 +401,21 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>  
>  		while (acc_sip > 0 || gyro_sip > 0 || ext_sip > 0) {
>  			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
> -				memcpy(gyro_buff, &hw->buff[offset],
> -				       ST_LSM6DSX_SAMPLE_SIZE);
> +				memcpy(hw->gyro_scan.channels,
> +				       &hw->buff[offset],
> +				       sizeof(hw->gyro_scan.channels));
>  				offset += ST_LSM6DSX_SAMPLE_SIZE;

what about doing?

offset += sizeof(hw->gyro_scan.channels)

it seems easier to follow, what do you think?

>  			}
>  			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
> -				memcpy(acc_buff, &hw->buff[offset],
> -				       ST_LSM6DSX_SAMPLE_SIZE);
> +				memcpy(hw->acc_scan.channels,
> +				       &hw->buff[offset],
> +				       sizeof(hw->acc_scan.channels));
>  				offset += ST_LSM6DSX_SAMPLE_SIZE;

same here

>  			}
>  			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
> -				memcpy(ext_buff, &hw->buff[offset],
> -				       ST_LSM6DSX_SAMPLE_SIZE);
> +				memcpy(hw->ext_scan.channels,
> +				       &hw->buff[offset],
> +				       sizeof(hw->ext_scan.channels));
>  				offset += ST_LSM6DSX_SAMPLE_SIZE;

same here

Regards,
Lorenzo

>  			}
>  
> @@ -446,19 +446,22 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>  			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
>  				iio_push_to_buffers_with_timestamp(
>  					hw->iio_devs[ST_LSM6DSX_ID_GYRO],
> -					gyro_buff, gyro_sensor->ts_ref + ts);
> +					&hw->gyro_scan,
> +					gyro_sensor->ts_ref + ts);
>  				gyro_sip--;
>  			}
>  			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
>  				iio_push_to_buffers_with_timestamp(
>  					hw->iio_devs[ST_LSM6DSX_ID_ACC],
> -					acc_buff, acc_sensor->ts_ref + ts);
> +					&hw->acc_scan,
> +					acc_sensor->ts_ref + ts);
>  				acc_sip--;
>  			}
>  			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
>  				iio_push_to_buffers_with_timestamp(
>  					hw->iio_devs[ST_LSM6DSX_ID_EXT0],
> -					ext_buff, ext_sensor->ts_ref + ts);
> +					&hw->ext_scan,
> +					ext_sensor->ts_ref + ts);
>  				ext_sip--;
>  			}
>  			sip++;
> -- 
> 2.26.2
>
Jonathan Cameron June 7, 2020, 3:07 p.m. UTC | #2
On Tue, 26 May 2020 09:58:38 +0200
Lorenzo Bianconi <lorenzo@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 set of suitable structures in the iio_priv() data.
> > 
> > This data is allocated with kzalloc so no data can leak apart from
> > previous readings.
> > 
> > There has been a lot of churn in this driver, so likely backports
> > may be needed for stable.
> > 
> > Fixes: 290a6ce11d93 ("iio: imu: add support to lsm6dsx driver")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  5 ++++
> >  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 27 ++++++++++---------
> >  2 files changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > index b56df409ed0f..5bc724eadc83 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > @@ -411,6 +411,11 @@ struct st_lsm6dsx_hw {
> >  	const struct st_lsm6dsx_settings *settings;
> >  
> >  	struct iio_mount_matrix orientation;
> > +	/* Ensure natural alignment of buffer elements */
> > +	struct {
> > +		__le16 channels[3];
> > +		s64 ts;
> > +	} gyro_scan, acc_scan, ext_scan;
> >  };
> >  
> >  static __maybe_unused const struct iio_event_spec st_lsm6dsx_event = {
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index afd00daeefb2..9bcffbfac797 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -341,9 +341,6 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >  	int err, sip, acc_sip, gyro_sip, ts_sip, ext_sip, read_len, offset;
> >  	u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
> >  	u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
> > -	u8 gyro_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
> > -	u8 acc_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
> > -	u8 ext_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
> >  	bool reset_ts = false;
> >  	__le16 fifo_status;
> >  	s64 ts = 0;
> > @@ -404,18 +401,21 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >  
> >  		while (acc_sip > 0 || gyro_sip > 0 || ext_sip > 0) {
> >  			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
> > -				memcpy(gyro_buff, &hw->buff[offset],
> > -				       ST_LSM6DSX_SAMPLE_SIZE);
> > +				memcpy(hw->gyro_scan.channels,
> > +				       &hw->buff[offset],
> > +				       sizeof(hw->gyro_scan.channels));
> >  				offset += ST_LSM6DSX_SAMPLE_SIZE;  
> 
> what about doing?
> 
> offset += sizeof(hw->gyro_scan.channels)
> 
> it seems easier to follow, what do you think?
> 
> >  			}
> >  			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
> > -				memcpy(acc_buff, &hw->buff[offset],
> > -				       ST_LSM6DSX_SAMPLE_SIZE);
> > +				memcpy(hw->acc_scan.channels,
> > +				       &hw->buff[offset],
> > +				       sizeof(hw->acc_scan.channels));
> >  				offset += ST_LSM6DSX_SAMPLE_SIZE;  
> 
> same here
> 
> >  			}
> >  			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
> > -				memcpy(ext_buff, &hw->buff[offset],
> > -				       ST_LSM6DSX_SAMPLE_SIZE);
> > +				memcpy(hw->ext_scan.channels,
> > +				       &hw->buff[offset],
> > +				       sizeof(hw->ext_scan.channels));
> >  				offset += ST_LSM6DSX_SAMPLE_SIZE;  
> 
> same here

Agreed.  I also missed the tagged path needs alignment.  For that one I did it
directly on the stack.  Please take a look at v2 and let me know if it looks
right to you.

Thanks,

Jonathan

> 
> Regards,
> Lorenzo
> 
> >  			}
> >  
> > @@ -446,19 +446,22 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >  			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
> >  				iio_push_to_buffers_with_timestamp(
> >  					hw->iio_devs[ST_LSM6DSX_ID_GYRO],
> > -					gyro_buff, gyro_sensor->ts_ref + ts);
> > +					&hw->gyro_scan,
> > +					gyro_sensor->ts_ref + ts);
> >  				gyro_sip--;
> >  			}
> >  			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
> >  				iio_push_to_buffers_with_timestamp(
> >  					hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > -					acc_buff, acc_sensor->ts_ref + ts);
> > +					&hw->acc_scan,
> > +					acc_sensor->ts_ref + ts);
> >  				acc_sip--;
> >  			}
> >  			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
> >  				iio_push_to_buffers_with_timestamp(
> >  					hw->iio_devs[ST_LSM6DSX_ID_EXT0],
> > -					ext_buff, ext_sensor->ts_ref + ts);
> > +					&hw->ext_scan,
> > +					ext_sensor->ts_ref + ts);
> >  				ext_sip--;
> >  			}
> >  			sip++;
> > -- 
> > 2.26.2
> >

Patch
diff mbox series

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index b56df409ed0f..5bc724eadc83 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -411,6 +411,11 @@  struct st_lsm6dsx_hw {
 	const struct st_lsm6dsx_settings *settings;
 
 	struct iio_mount_matrix orientation;
+	/* Ensure natural alignment of buffer elements */
+	struct {
+		__le16 channels[3];
+		s64 ts;
+	} gyro_scan, acc_scan, ext_scan;
 };
 
 static __maybe_unused const struct iio_event_spec st_lsm6dsx_event = {
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index afd00daeefb2..9bcffbfac797 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -341,9 +341,6 @@  int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
 	int err, sip, acc_sip, gyro_sip, ts_sip, ext_sip, read_len, offset;
 	u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
 	u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
-	u8 gyro_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
-	u8 acc_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
-	u8 ext_buff[ST_LSM6DSX_IIO_BUFF_SIZE];
 	bool reset_ts = false;
 	__le16 fifo_status;
 	s64 ts = 0;
@@ -404,18 +401,21 @@  int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
 
 		while (acc_sip > 0 || gyro_sip > 0 || ext_sip > 0) {
 			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
-				memcpy(gyro_buff, &hw->buff[offset],
-				       ST_LSM6DSX_SAMPLE_SIZE);
+				memcpy(hw->gyro_scan.channels,
+				       &hw->buff[offset],
+				       sizeof(hw->gyro_scan.channels));
 				offset += ST_LSM6DSX_SAMPLE_SIZE;
 			}
 			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
-				memcpy(acc_buff, &hw->buff[offset],
-				       ST_LSM6DSX_SAMPLE_SIZE);
+				memcpy(hw->acc_scan.channels,
+				       &hw->buff[offset],
+				       sizeof(hw->acc_scan.channels));
 				offset += ST_LSM6DSX_SAMPLE_SIZE;
 			}
 			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
-				memcpy(ext_buff, &hw->buff[offset],
-				       ST_LSM6DSX_SAMPLE_SIZE);
+				memcpy(hw->ext_scan.channels,
+				       &hw->buff[offset],
+				       sizeof(hw->ext_scan.channels));
 				offset += ST_LSM6DSX_SAMPLE_SIZE;
 			}
 
@@ -446,19 +446,22 @@  int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
 			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
 				iio_push_to_buffers_with_timestamp(
 					hw->iio_devs[ST_LSM6DSX_ID_GYRO],
-					gyro_buff, gyro_sensor->ts_ref + ts);
+					&hw->gyro_scan,
+					gyro_sensor->ts_ref + ts);
 				gyro_sip--;
 			}
 			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
 				iio_push_to_buffers_with_timestamp(
 					hw->iio_devs[ST_LSM6DSX_ID_ACC],
-					acc_buff, acc_sensor->ts_ref + ts);
+					&hw->acc_scan,
+					acc_sensor->ts_ref + ts);
 				acc_sip--;
 			}
 			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
 				iio_push_to_buffers_with_timestamp(
 					hw->iio_devs[ST_LSM6DSX_ID_EXT0],
-					ext_buff, ext_sensor->ts_ref + ts);
+					&hw->ext_scan,
+					ext_sensor->ts_ref + ts);
 				ext_sip--;
 			}
 			sip++;