Message ID | 20200722155103.979802-4-jic23@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | IIO: Fused set 1 and 2 of timestamp alignment fixes | expand |
On Wed, 2020-07-22 at 16:50 +0100, Jonathan Cameron 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 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 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. > > Fixes tag is beyond some major refactoring so likely manual > backporting > would be needed to get that far back. > > Whilst the force alignment of the ts is not strictly necessary, it > does make the code less fragile. > > Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware > fifo") > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/accel/bmc150-accel-core.c > b/drivers/iio/accel/bmc150-accel-core.c > index 24864d9dfab5..48435865fdaf 100644 > --- a/drivers/iio/accel/bmc150-accel-core.c > +++ b/drivers/iio/accel/bmc150-accel-core.c > @@ -189,6 +189,14 @@ struct bmc150_accel_data { > struct mutex mutex; > u8 fifo_mode, watermark; > s16 buffer[8]; > + /* > + * Ensure there is sufficient space and correct alignment for > + * the timestamp if enabled > + */ > + struct { > + __le16 channels[3]; > + s64 ts __aligned(8); > + } scan; > u8 bw_bits; > u32 slope_dur; > u32 slope_thres; > @@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct > iio_dev *indio_dev, > * now. > */ > for (i = 0; i < count; i++) { > - u16 sample[8]; > int j, bit; > > j = 0; > for_each_set_bit(bit, indio_dev->active_scan_mask, > indio_dev->masklength) > - memcpy(&sample[j++], &buffer[i * 3 + bit], 2); > + memcpy(&data->scan.channels[j++], &buffer[i * 3 > + bit], > + sizeof(data->scan.channels[0])); > > - iio_push_to_buffers_with_timestamp(indio_dev, sample, > tstamp); > + iio_push_to_buffers_with_timestamp(indio_dev, &data- > >scan, > + tstamp); > > tstamp += sample_period; > }
On Wed, 29 Jul 2020 10:12:36 -0700 Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Wed, 2020-07-22 at 16:50 +0100, Jonathan Cameron 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 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 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. > > > > Fixes tag is beyond some major refactoring so likely manual > > backporting > > would be needed to get that far back. > > > > Whilst the force alignment of the ts is not strictly necessary, it > > does make the code less fragile. > > > > Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware > > fifo") > > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > > > --- > > drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/accel/bmc150-accel-core.c > > b/drivers/iio/accel/bmc150-accel-core.c > > index 24864d9dfab5..48435865fdaf 100644 > > --- a/drivers/iio/accel/bmc150-accel-core.c > > +++ b/drivers/iio/accel/bmc150-accel-core.c > > @@ -189,6 +189,14 @@ struct bmc150_accel_data { > > struct mutex mutex; > > u8 fifo_mode, watermark; > > s16 buffer[8]; > > + /* > > + * Ensure there is sufficient space and correct alignment for > > + * the timestamp if enabled > > + */ > > + struct { > > + __le16 channels[3]; > > + s64 ts __aligned(8); > > + } scan; > > u8 bw_bits; > > u32 slope_dur; > > u32 slope_thres; > > @@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct > > iio_dev *indio_dev, > > * now. > > */ > > for (i = 0; i < count; i++) { > > - u16 sample[8]; > > int j, bit; > > > > j = 0; > > for_each_set_bit(bit, indio_dev->active_scan_mask, > > indio_dev->masklength) > > - memcpy(&sample[j++], &buffer[i * 3 + bit], 2); > > + memcpy(&data->scan.channels[j++], &buffer[i * 3 > > + bit], > > + sizeof(data->scan.channels[0])); > > > > - iio_push_to_buffers_with_timestamp(indio_dev, sample, > > tstamp); > > + iio_push_to_buffers_with_timestamp(indio_dev, &data- > > >scan, > > + tstamp); > > > > tstamp += sample_period; > > } >
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c index 24864d9dfab5..48435865fdaf 100644 --- a/drivers/iio/accel/bmc150-accel-core.c +++ b/drivers/iio/accel/bmc150-accel-core.c @@ -189,6 +189,14 @@ struct bmc150_accel_data { struct mutex mutex; u8 fifo_mode, watermark; s16 buffer[8]; + /* + * Ensure there is sufficient space and correct alignment for + * the timestamp if enabled + */ + struct { + __le16 channels[3]; + s64 ts __aligned(8); + } scan; u8 bw_bits; u32 slope_dur; u32 slope_thres; @@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev, * now. */ for (i = 0; i < count; i++) { - u16 sample[8]; int j, bit; j = 0; for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) - memcpy(&sample[j++], &buffer[i * 3 + bit], 2); + memcpy(&data->scan.channels[j++], &buffer[i * 3 + bit], + sizeof(data->scan.channels[0])); - iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp); + iio_push_to_buffers_with_timestamp(indio_dev, &data->scan, + tstamp); tstamp += sample_period; }