| Message ID | 20200722155103.979802-28-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, 22 Jul 2020 16:51:03 +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 apart > from previous readings. > > The explicit alignment of ts is necessary to ensure correct padding > on architectures where s64 is only 4 bytes aligned such as x86_32. > > Fixes: a9e9c7153e96 ("iio: adc: add max1117/max1118/max1119 ADC driver") > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Cc: Akinobu Mita <akinobu.mita@gmail.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Applied and marked for stable. Thanks, Jonathan > --- > drivers/iio/adc/max1118.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/adc/max1118.c b/drivers/iio/adc/max1118.c > index 01b20e420ac4..6efb0b43d938 100644 > --- a/drivers/iio/adc/max1118.c > +++ b/drivers/iio/adc/max1118.c > @@ -36,6 +36,11 @@ struct max1118 { > struct spi_device *spi; > struct mutex lock; > struct regulator *reg; > + /* Ensure natural alignment of buffer elements */ > + struct { > + u8 channels[2]; > + s64 ts __aligned(8); > + } scan; > > u8 data ____cacheline_aligned; > }; > @@ -166,7 +171,6 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p) > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > struct max1118 *adc = iio_priv(indio_dev); > - u8 data[16] = { }; /* 2x 8-bit ADC data + padding + 8 bytes timestamp */ > int scan_index; > int i = 0; > > @@ -184,10 +188,10 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p) > goto out; > } > > - data[i] = ret; > + adc->scan.channels[i] = ret; > i++; > } > - iio_push_to_buffers_with_timestamp(indio_dev, data, > + iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, > iio_get_time_ns(indio_dev)); > out: > mutex_unlock(&adc->lock);
diff --git a/drivers/iio/adc/max1118.c b/drivers/iio/adc/max1118.c index 01b20e420ac4..6efb0b43d938 100644 --- a/drivers/iio/adc/max1118.c +++ b/drivers/iio/adc/max1118.c @@ -36,6 +36,11 @@ struct max1118 { struct spi_device *spi; struct mutex lock; struct regulator *reg; + /* Ensure natural alignment of buffer elements */ + struct { + u8 channels[2]; + s64 ts __aligned(8); + } scan; u8 data ____cacheline_aligned; }; @@ -166,7 +171,6 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p) struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; struct max1118 *adc = iio_priv(indio_dev); - u8 data[16] = { }; /* 2x 8-bit ADC data + padding + 8 bytes timestamp */ int scan_index; int i = 0; @@ -184,10 +188,10 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p) goto out; } - data[i] = ret; + adc->scan.channels[i] = ret; i++; } - iio_push_to_buffers_with_timestamp(indio_dev, data, + iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, iio_get_time_ns(indio_dev)); out: mutex_unlock(&adc->lock);