> From: Jonathan Cameron > > 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. > > For the tagged path the data is aligned by using __aligned(8) for > the buffer on the stack. > > There has been a lot of churn in this driver, so likely backports > may be needed for stable. Hi Jonathan, I added just some nitpicks inline, but it seems to me the patch is fine. I guess we can address them with a followup patch if you agree, no need to resend this huge series :) Acked-by: Lorenzo Bianconi > > Fixes: 290a6ce11d93 ("iio: imu: add support to lsm6dsx driver") > Reported-by: Lars-Peter Clausen > Cc: Lorenzo Bianconi > Signed-off-by: Jonathan Cameron > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 5 +++ > .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 36 ++++++++++--------- > 2 files changed, 25 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h > index b56df409ed0f..5f821ef467da 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 __aligned(8); > + } gyro_scan, acc_scan, ext_scan; > }; it seems to me doing something like: struct { __le16 channels[3]; s64 ts __aligned(8); } scan[3]; would be better if for example we want to add support for more external devices for untagged FIFO devices > > 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..bebbc2bb37f7 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,19 +401,22 @@ 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); > - offset += ST_LSM6DSX_SAMPLE_SIZE; > + memcpy(hw->gyro_scan.channels, > + &hw->buff[offset], > + sizeof(hw->gyro_scan.channels)); > + offset += sizeof(hw->gyro_scan.channels); > } > if (acc_sip > 0 && !(sip % acc_sensor->decimator)) { > - memcpy(acc_buff, &hw->buff[offset], > - ST_LSM6DSX_SAMPLE_SIZE); > - offset += ST_LSM6DSX_SAMPLE_SIZE; > + memcpy(hw->acc_scan.channels, > + &hw->buff[offset], > + sizeof(hw->acc_scan.channels)); > + offset += sizeof(hw->acc_scan.channels); > } > if (ext_sip > 0 && !(sip % ext_sensor->decimator)) { > - memcpy(ext_buff, &hw->buff[offset], > - ST_LSM6DSX_SAMPLE_SIZE); > - offset += ST_LSM6DSX_SAMPLE_SIZE; > + memcpy(hw->ext_scan.channels, > + &hw->buff[offset], > + sizeof(hw->ext_scan.channels)); > + offset += sizeof(hw->ext_scan.channels); > } > > if (ts_sip-- > 0) { > @@ -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++; > @@ -543,7 +546,8 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw) > { > u16 pattern_len = hw->sip * ST_LSM6DSX_TAGGED_SAMPLE_SIZE; > u16 fifo_len, fifo_diff_mask; > - u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE], tag; > + u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8); here we can use hw->scan[0] and drop the array on the stack > + u8 tag; > bool reset_ts = false; > int i, err, read_len; > __le16 fifo_status; > -- > 2.26.2 >