[PATCH 00/25] IIO: 2nd set of timestamp alignment fixes.

[PATCH 00/25] IIO: 2nd set of timestamp alignment fixes.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Note there are still quite a few in the first set [2] that I have not
picked up yet due to lack of review.   Any help with sanity checking
those (and these) would be much appreciated.  These are not quite
mechanical enough for me to be sure I haven't done anything silly
so the more eyes the better.  Also see that discussion for extra
details on why I went in various directions to fix this in the
different drivers.

I've mostly avoided simply using __aligned(8) on the stack because
it is only relatively recently that that has been guaranteed to work.
Gcc 4.6 and kernel 4.19.  I'd like it to be sensible to backport
most of these without significant rework. The mpl3115 is I think the
only exception.

I also snuck in a warning fix for endian casting as it was in the code
that I was touching to do the main fix.

So what's left from Lars list? 5 harder cases where I'm either fairly
sure there are other bugs in the code, or I simply couldn't face
figuring them out today.

Anyhow, to recap - round 1 cover letter.

Lars noted in a recent review [1] of the adis16475 that we had an issue around
the alignment requirements of iio_push_to_buffers_with_timestamp.
Whilst it's not documented, that function assumes that the overall buffer
is 8 byte aligned, to ensure the timestamp is itself naturally aligned.
We have drivers that use arrays (typically on the stack) that do
not guarantee this alignment.

We could have fixed this by using a put_unaligned to write the timestamp
but I think that just pushes the problem down the line.  If we were to
have a consumer buffer wanting all the channels in the current
active_scanmask then it will get the raw buffer from the driver passed
straight through.  It seems odd to me if we allow passing a buffer
that is not naturally aligned through to a consumer.
Hence I'm proposing to fix up all existing drivers that might pass
a buffer with insufficient alignment guarantees.
Sometimes the timestamp is guaranteed to be in a particular location,
in which case we can use C structure alignment guarantees to fix this
in a nice readable fashion.  In other cases, the timestamp location
depends on which channels are enabled, and in those case we can
use explicit alignment __aligned(8) to ensure the whole array is
appropriately aligned.

Lars-Peter also noted that, in many of these cases, there are holes
in the stack array that we never write.  Those provide a potential
leak of kernel data to userspace.  For drivers where this applies
we either need to zero those holes each time, or allocate the buffer
on the heap (only once), ensuring it is zeroed at that time.
We may leak previous values from the sensor but currently that seems
unlikely to present any form of security risk.

As such, this first set contains a mixture of fixes.  Where there
are no possible holes, the buffer is kept on the stack but a
c structure is used to guarantee appropriate alignment.  Where
there are holes, the buffer is moved into the iio_priv() accessed
data private structure. A c structure or __aligned(8) is used
as appropriate.

I've stopped at this point rather than doing all the drivers Lars
found in order to both throttle the review burden and also to
see find any general problems with the fixes before doign futher
similar series.  A few of the remaining ones will be rather more
complex to deal with.

These have been there a long time, so whilst they are fixes we
will want in stable I'm not that bothered if it takes us a little
while to get them there!

[1] https://www.spinics.net/lists/devicetree/msg350590.html
[2] https://patchwork.kernel.org/cover/11554215/

Jonathan Cameron (25):
  iio:light:si1145: Fix timestamp alignment and prevent data leak.
  iio:light:max44000 Fix timestamp alignment and prevent data leak.
  iio:light:rpr0521 Fix timestamp alignment and prevent data leak.
  iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.
  iio:light:ltr501 Fix timestamp alignment issue.
  iio:magnetometer:ak8974: Fix alignment and data leak issues
  iio:magnetometer:ak8975 Fix alignment and data leak issues.
  iio:magnetometer:mag3110 Fix alignment and data leak issues.
  iio:humidity:hdc100x Fix alignment and data leak issues
  iio:humidity:hts221 Fix alignment and data leak issues
  iio:imu:bmi160 Fix alignment and data leak issues
  iio:imu:st_lsm6dsx Fix alignment and data leak issues
  iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.
  iio:pressure:ms5611 Fix buffer element alignment
  iio:pressure:mpl3115 Force alignment of buffer
  iio:adc:ti-adc081c Fix alignment and data leak issues
  iio:adc:ti-adc084s021 Fix alignment and data leak issues.
  iio:adc:ti-adc084s021 Tidy up endian types
  iio:adc:ti-ads1015 Fix buffer element alignment
  iio:adc:ti-ads124s08 Fix alignment and data leak issues.
  iio:adc:ti-ads8688 Fix alignment and potential data leak issue
  iio:adc:ti-adc0832 Fix alignment issue with timestamp
  iio:adc:ti-adc12138 Fix alignment issue with timestamp
  iio:adc:ina2xx Fix timestamp alignment issue.
  iio:adc:max1118 Fix alignment of timestamp and data leak issues

 drivers/iio/adc/ina2xx-adc.c                  |  8 +++---
 drivers/iio/adc/max1118.c                     | 10 ++++---
 drivers/iio/adc/ti-adc081c.c                  | 11 +++++---
 drivers/iio/adc/ti-adc0832.c                  |  7 ++---
 drivers/iio/adc/ti-adc084s021.c               | 20 ++++++++------
 drivers/iio/adc/ti-adc12138.c                 |  9 ++++---
 drivers/iio/adc/ti-ads1015.c                  | 12 ++++++---
 drivers/iio/adc/ti-ads124s08.c                | 10 ++++---
 drivers/iio/adc/ti-ads8688.c                  | 12 ++++++---
 drivers/iio/humidity/hdc100x.c                | 10 ++++---
 drivers/iio/humidity/hts221.h                 |  5 ++++
 drivers/iio/humidity/hts221_buffer.c          |  9 ++++---
 drivers/iio/imu/bmi160/bmi160.h               |  2 ++
 drivers/iio/imu/bmi160/bmi160_core.c          |  5 ++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  8 +++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 12 ++++-----
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  5 ++++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 27 ++++++++++---------
 drivers/iio/light/ltr501.c                    | 15 ++++++-----
 drivers/iio/light/max44000.c                  | 12 ++++++---
 drivers/iio/light/rpr0521.c                   | 17 +++++++++---
 drivers/iio/light/si1145.c                    |  7 ++---
 drivers/iio/light/st_uvis25.h                 |  5 ++++
 drivers/iio/light/st_uvis25_core.c            |  6 ++---
 drivers/iio/magnetometer/ak8974.c             | 10 ++++---
 drivers/iio/magnetometer/ak8975.c             | 20 +++++++++-----
 drivers/iio/magnetometer/mag3110.c            | 13 ++++++---
 drivers/iio/pressure/mpl3115.c                |  3 ++-
 drivers/iio/pressure/ms5611_core.c            | 11 +++++---
 29 files changed, 198 insertions(+), 103 deletions(-)

-- 
2.26.2

[PATCH 01/25] iio:light:si1145: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

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 24 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 array in the iio_priv() data with alignment
explicitly requested.  This data is allocated with kzalloc so no
data can leak appart from previous readings.

Fixes: ac45e57f1590 ("iio: light: Add driver for Silabs si1132, si1141/2/3 and si1145/6/7 ambient light, uv index and proximity sensors")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
---
 drivers/iio/light/si1145.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
index 0476c2bc8138..aa93c6d38043 100644
--- a/drivers/iio/light/si1145.c
+++ b/drivers/iio/light/si1145.c
@@ -179,6 +179,8 @@ struct si1145_data {
 	bool autonomous;
 	struct iio_trigger *trig;
 	int meas_rate;
+	/* Ensure timestamp will be naturally aligned if present */
+	u8 buffer[24] __aligned(8);
 };
 
 /**
@@ -445,7 +447,6 @@ static irqreturn_t si1145_trigger_handler(int irq, void *private)
 	 *   6*2 bytes channels data + 4 bytes alignment +
 	 *   8 bytes timestamp
 	 */
-	u8 buffer[24];
 	int i, j = 0;
 	int ret;
 	u8 irq_status = 0;
@@ -478,7 +479,7 @@ static irqreturn_t si1145_trigger_handler(int irq, void *private)
 
 		ret = i2c_smbus_read_i2c_block_data_or_emulated(
 				data->client, indio_dev->channels[i].address,
-				sizeof(u16) * run, &buffer[j]);
+				sizeof(u16) * run, &data->buffer[j]);
 		if (ret < 0)
 			goto done;
 		j += run * sizeof(u16);
@@ -493,7 +494,7 @@ static irqreturn_t si1145_trigger_handler(int irq, void *private)
 			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:
-- 
2.26.2

[PATCH 02/25] iio:light:max44000 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

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().
This data is allocated with kzalloc so no data can leak appart
from previous readings.

Fixes: 06ad7ea10e2b ("max44000: Initial triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/light/max44000.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index d6d8007ba430..d27b6170792d 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -75,6 +75,11 @@
 struct max44000_data {
 	struct mutex lock;
 	struct regmap *regmap;
+	/* Ensure naturally aligned timestamp */
+	struct {
+		u16 channels[2];
+		s64 ts;
+	} scan;
 };
 
 /* Default scale is set to the minimum of 0.03125 or 1 / (1 << 5) lux */
@@ -488,7 +493,6 @@ static irqreturn_t max44000_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct max44000_data *data = iio_priv(indio_dev);
-	u16 buf[8]; /* 2x u16 + padding + 8 bytes timestamp */
 	int index = 0;
 	unsigned int regval;
 	int ret;
@@ -498,17 +502,17 @@ static irqreturn_t max44000_trigger_handler(int irq, void *p)
 		ret = max44000_read_alsval(data);
 		if (ret < 0)
 			goto out_unlock;
-		buf[index++] = ret;
+		data->scan.channels[index++] = ret;
 	}
 	if (test_bit(MAX44000_SCAN_INDEX_PRX, indio_dev->active_scan_mask)) {
 		ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, &regval);
 		if (ret < 0)
 			goto out_unlock;
-		buf[index] = regval;
+		data->scan.channels[index] = regval;
 	}
 	mutex_unlock(&data->lock);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
 					   iio_get_time_ns(indio_dev));
 	iio_trigger_notify_done(indio_dev->trig);
 	return IRQ_HANDLED;
-- 
2.26.2

[PATCH 03/25] iio:light:rpr0521 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

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().
This data is allocated with kzalloc so no data can leak appart
from previous readings and in this case the status byte from the device.

Fixes: e12ffd241c00 ("iio: light: rpr0521 triggered buffer")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index a0a7aeae5a82..a1fc8a91b2b6 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -194,6 +194,17 @@ struct rpr0521_data {
 	bool pxs_need_dis;
 
 	struct regmap *regmap;
+
+	/*
+	 * Ensure correct naturally aligned timestamp.
+	 * Note that the read will put garbage data into
+	 * the padding but this should not be a problem
+	 */
+	struct {
+		__le16 channels[3];
+		u8 garbage;
+		s64 ts;
+	} scan;
 };
 
 static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
@@ -449,8 +460,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 	struct rpr0521_data *data = iio_priv(indio_dev);
 	int err;
 
-	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
-
 	/* Use irq timestamp when reasonable. */
 	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
 		pf->timestamp = data->irq_timestamp;
@@ -461,11 +470,11 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 		pf->timestamp = iio_get_time_ns(indio_dev);
 
 	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
-		&buffer,
+		data->scan.channels,
 		(3 * 2) + 1);	/* 3 * 16-bit + (discarded) int clear reg. */
 	if (!err)
 		iio_push_to_buffers_with_timestamp(indio_dev,
-						   buffer, pf->timestamp);
+						   &data->scan, pf->timestamp);
 	else
 		dev_err(&data->client->dev,
 			"Trigger consumer can't read from sensor.\n");
-- 
2.26.2

[PATCH 04/25] iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

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()

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

Fixes: 3025c8688c1e ("iio: light: add support for UVIS25 sensor")
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/light/st_uvis25.h      | 5 +++++
 drivers/iio/light/st_uvis25_core.c | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/st_uvis25.h b/drivers/iio/light/st_uvis25.h
index 78bc56aad129..f7027e4c4493 100644
--- a/drivers/iio/light/st_uvis25.h
+++ b/drivers/iio/light/st_uvis25.h
@@ -27,6 +27,11 @@ struct st_uvis25_hw {
 	struct iio_trigger *trig;
 	bool enabled;
 	int irq;
+	/* Ensure timestamp is naturally aligned */
+	struct {
+		u8 chan;
+		s64 ts;
+	} scan;
 };
 
 extern const struct dev_pm_ops st_uvis25_pm_ops;
diff --git a/drivers/iio/light/st_uvis25_core.c b/drivers/iio/light/st_uvis25_core.c
index d262c254b895..fe1f2dc970c7 100644
--- a/drivers/iio/light/st_uvis25_core.c
+++ b/drivers/iio/light/st_uvis25_core.c
@@ -234,17 +234,17 @@ static const struct iio_buffer_setup_ops st_uvis25_buffer_ops = {
 
 static irqreturn_t st_uvis25_buffer_handler_thread(int irq, void *p)
 {
-	u8 buffer[ALIGN(sizeof(u8), sizeof(s64)) + sizeof(s64)];
 	struct iio_poll_func *pf = p;
 	struct iio_dev *iio_dev = pf->indio_dev;
 	struct st_uvis25_hw *hw = iio_priv(iio_dev);
 	int err;
 
-	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR, (int *)buffer);
+	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR,
+			  (unsigned int *)&hw->scan.chan);
 	if (err < 0)
 		goto out;
 
-	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
+	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
 					   iio_get_time_ns(iio_dev));
 
 out:
-- 
2.26.2

[PATCH 05/25] iio:light:ltr501 Fix timestamp alignment issue.

[Jonathan Cameron]

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.
Here we use a structure on the stack.  The driver already did an
explicit memset so no data leak was possible.

Note there has been some rework in this driver of the years, so no
way this will apply cleanly all the way back.

Fixes: 2690be905123 ("iio: Add Lite-On ltr501 ambient light / proximity sensor driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/light/ltr501.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 5a3fcb127cd2..c8b1ca13eb55 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -1243,13 +1243,16 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ltr501_data *data = iio_priv(indio_dev);
-	u16 buf[8];
+	struct {
+		u16 channels[3];
+		s64 ts;
+	} scan;
 	__le16 als_buf[2];
 	u8 mask = 0;
 	int j = 0;
 	int ret, psdata;
 
-	memset(buf, 0, sizeof(buf));
+	memset(&scan, 0, sizeof(scan));
 
 	/* figure out which data needs to be ready */
 	if (test_bit(0, indio_dev->active_scan_mask) ||
@@ -1268,9 +1271,9 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
 		if (ret < 0)
 			return ret;
 		if (test_bit(0, indio_dev->active_scan_mask))
-			buf[j++] = le16_to_cpu(als_buf[1]);
+			scan.channels[j++] = le16_to_cpu(als_buf[1]);
 		if (test_bit(1, indio_dev->active_scan_mask))
-			buf[j++] = le16_to_cpu(als_buf[0]);
+			scan.channels[j++] = le16_to_cpu(als_buf[0]);
 	}
 
 	if (mask & LTR501_STATUS_PS_RDY) {
@@ -1278,10 +1281,10 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
 				       &psdata, 2);
 		if (ret < 0)
 			goto done;
-		buf[j++] = psdata & LTR501_PS_DATA_MASK;
+		scan.channels[j++] = psdata & LTR501_PS_DATA_MASK;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
 					   iio_get_time_ns(indio_dev));
 
 done:
-- 
2.26.2

[PATCH 06/25] iio:magnetometer:ak8974: Fix alignment and data leak issues

[Jonathan Cameron]

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 appart from
previous readings.

Fixes: 7c94a8b2ee8cf ("iio: magn: add a driver for AK8974")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/magnetometer/ak8974.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index 810fdfd37c88..899795add957 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -192,6 +192,11 @@ struct ak8974 {
 	bool drdy_irq;
 	struct completion drdy_complete;
 	bool drdy_active_low;
+	/* Ensure timestamp is naturally aligned */
+	struct {
+		__le16 channels[3];
+		s64 ts;
+	} scan;
 };
 
 static const char ak8974_reg_avdd[] = "avdd";
@@ -657,7 +662,6 @@ static void ak8974_fill_buffer(struct iio_dev *indio_dev)
 {
 	struct ak8974 *ak8974 = iio_priv(indio_dev);
 	int ret;
-	__le16 hw_values[8]; /* Three axes + 64bit padding */
 
 	pm_runtime_get_sync(&ak8974->i2c->dev);
 	mutex_lock(&ak8974->lock);
@@ -667,13 +671,13 @@ static void ak8974_fill_buffer(struct iio_dev *indio_dev)
 		dev_err(&ak8974->i2c->dev, "error triggering measure\n");
 		goto out_unlock;
 	}
-	ret = ak8974_getresult(ak8974, hw_values);
+	ret = ak8974_getresult(ak8974, ak8974->scan.channels);
 	if (ret) {
 		dev_err(&ak8974->i2c->dev, "error getting measures\n");
 		goto out_unlock;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, hw_values,
+	iio_push_to_buffers_with_timestamp(indio_dev, &ak8974->scan,
 					   iio_get_time_ns(indio_dev));
 
  out_unlock:
-- 
2.26.2

[PATCH 07/25] iio:magnetometer:ak8975 Fix alignment and data leak issues.

[Jonathan Cameron]

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.

Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Gregor Boirie <gregor.boirie@parrot.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/magnetometer/ak8975.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 3c881541ae72..673ac70d014d 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -365,6 +365,12 @@ struct ak8975_data {
 	struct iio_mount_matrix orientation;
 	struct regulator	*vdd;
 	struct regulator	*vid;
+
+	/* Ensure natural alignment of timestamp */
+	struct {
+		s16 channels[3];
+		s64 ts;
+	} scan;
 };
 
 /* Enable attached power regulator if any. */
@@ -787,7 +793,6 @@ static void ak8975_fill_buffer(struct iio_dev *indio_dev)
 	const struct i2c_client *client = data->client;
 	const struct ak_def *def = data->def;
 	int ret;
-	s16 buff[8]; /* 3 x 16 bits axis values + 1 aligned 64 bits timestamp */
 	__le16 fval[3];
 
 	mutex_lock(&data->lock);
@@ -810,11 +815,14 @@ static void ak8975_fill_buffer(struct iio_dev *indio_dev)
 	mutex_unlock(&data->lock);
 
 	/* Clamp to valid range. */
-	buff[0] = clamp_t(s16, le16_to_cpu(fval[0]), -def->range, def->range);
-	buff[1] = clamp_t(s16, le16_to_cpu(fval[1]), -def->range, def->range);
-	buff[2] = clamp_t(s16, le16_to_cpu(fval[2]), -def->range, def->range);
-
-	iio_push_to_buffers_with_timestamp(indio_dev, buff,
+	data->scan.channels[0] =
+		clamp_t(s16, le16_to_cpu(fval[0]), -def->range, def->range);
+	data->scan.channels[1] =
+		clamp_t(s16, le16_to_cpu(fval[1]), -def->range, def->range);
+	data->scan.channels[2] =
+		clamp_t(s16, le16_to_cpu(fval[2]), -def->range, def->range);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
 					   iio_get_time_ns(indio_dev));
 	return;
 
-- 
2.26.2

[PATCH 08/25] iio:magnetometer:mag3110 Fix alignment and data leak issues.

[Jonathan Cameron]

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.

Fixes: 39631b5f9584 ("iio: Add Freescale mag3110 magnetometer driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/magnetometer/mag3110.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/magnetometer/mag3110.c b/drivers/iio/magnetometer/mag3110.c
index fb16cfdd6fa6..17cd613c21c9 100644
--- a/drivers/iio/magnetometer/mag3110.c
+++ b/drivers/iio/magnetometer/mag3110.c
@@ -56,6 +56,12 @@ struct mag3110_data {
 	int sleep_val;
 	struct regulator *vdd_reg;
 	struct regulator *vddio_reg;
+	/* Ensure natural alignment of timestamp */
+	struct {
+		__be16 channels[3];
+		u8 temp;
+		s64 ts;
+	} scan;
 };
 
 static int mag3110_request(struct mag3110_data *data)
@@ -387,10 +393,9 @@ static irqreturn_t mag3110_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mag3110_data *data = iio_priv(indio_dev);
-	u8 buffer[16]; /* 3 16-bit channels + 1 byte temp + padding + ts */
 	int ret;
 
-	ret = mag3110_read(data, (__be16 *) buffer);
+	ret = mag3110_read(data, data->scan.channels);
 	if (ret < 0)
 		goto done;
 
@@ -399,10 +404,10 @@ static irqreturn_t mag3110_trigger_handler(int irq, void *p)
 			MAG3110_DIE_TEMP);
 		if (ret < 0)
 			goto done;
-		buffer[6] = ret;
+		data->scan.temp = ret;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
 		iio_get_time_ns(indio_dev));
 
 done:
-- 
2.26.2

[PATCH 09/25] iio:humidity:hdc100x Fix alignment and data leak issues

[Jonathan Cameron]

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.

Fixes: 16bf793f86b2 ("iio: humidity: hdc100x: add triggered buffer support for HDC100X")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alison Schofield <amsfield22@gmail.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/humidity/hdc100x.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index 7ecd2ffa3132..fd825e281d4f 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -38,6 +38,11 @@ struct hdc100x_data {
 
 	/* integration time of the sensor */
 	int adc_int_us[2];
+	/* Ensure natural alignment of timestamp */
+	struct {
+		__be16 channels[2];
+		s64 ts;
+	} scan;
 };
 
 /* integration time in us */
@@ -322,7 +327,6 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
 	struct i2c_client *client = data->client;
 	int delay = data->adc_int_us[0] + data->adc_int_us[1];
 	int ret;
-	s16 buf[8];  /* 2x s16 + padding + 8 byte timestamp */
 
 	/* dual read starts at temp register */
 	mutex_lock(&data->lock);
@@ -333,13 +337,13 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
 	}
 	usleep_range(delay, delay + 1000);
 
-	ret = i2c_master_recv(client, (u8 *)buf, 4);
+	ret = i2c_master_recv(client, (u8 *)data->scan.channels, 4);
 	if (ret < 0) {
 		dev_err(&client->dev, "cannot read sensor data\n");
 		goto err;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
 					   iio_get_time_ns(indio_dev));
 err:
 	mutex_unlock(&data->lock);
-- 
2.26.2

[PATCH 10/25] iio:humidity:hts221 Fix alignment and data leak issues

[Jonathan Cameron]

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.

Fixes: e4a70e3e7d84 ("iio: humidity: add support to hts221 rh/temp combo device")
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/humidity/hts221.h        | 5 +++++
 drivers/iio/humidity/hts221_buffer.c | 9 +++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
index 7c650df77556..6ad579ca9cef 100644
--- a/drivers/iio/humidity/hts221.h
+++ b/drivers/iio/humidity/hts221.h
@@ -39,6 +39,11 @@ struct hts221_hw {
 
 	bool enabled;
 	u8 odr;
+	/* Ensure natural alignment of timestamp */
+	struct {
+		__le16 channels[2];
+		s64 ts;
+	} scan;
 };
 
 extern const struct dev_pm_ops hts221_pm_ops;
diff --git a/drivers/iio/humidity/hts221_buffer.c b/drivers/iio/humidity/hts221_buffer.c
index 21c6c160462d..59ede9860185 100644
--- a/drivers/iio/humidity/hts221_buffer.c
+++ b/drivers/iio/humidity/hts221_buffer.c
@@ -160,7 +160,6 @@ static const struct iio_buffer_setup_ops hts221_buffer_ops = {
 
 static irqreturn_t hts221_buffer_handler_thread(int irq, void *p)
 {
-	u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)];
 	struct iio_poll_func *pf = p;
 	struct iio_dev *iio_dev = pf->indio_dev;
 	struct hts221_hw *hw = iio_priv(iio_dev);
@@ -170,18 +169,20 @@ static irqreturn_t hts221_buffer_handler_thread(int irq, void *p)
 	/* humidity data */
 	ch = &iio_dev->channels[HTS221_SENSOR_H];
 	err = regmap_bulk_read(hw->regmap, ch->address,
-			       buffer, HTS221_DATA_SIZE);
+			       &hw->scan.channels[0],
+			       sizeof(hw->scan.channels[0]));
 	if (err < 0)
 		goto out;
 
 	/* temperature data */
 	ch = &iio_dev->channels[HTS221_SENSOR_T];
 	err = regmap_bulk_read(hw->regmap, ch->address,
-			       buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE);
+			       &hw->scan.channels[1],
+			       sizeof(hw->scan.channels[1]));
 	if (err < 0)
 		goto out;
 
-	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
+	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
 					   iio_get_time_ns(iio_dev));
 
 out:
-- 
2.26.2

[PATCH 11/25] iio:imu:bmi160 Fix alignment and data leak issues

[Jonathan Cameron]

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 array in the iio_priv() data with alignment
explicitly requested.  This data is allocated with kzalloc so no
data can leak apart from previous readings.

Fixes: 77c4ad2d6a9b ("iio: imu: Add initial support for Bosch BMI160")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Daniel Baluta  <daniel.baluta@gmail.com>
---
 drivers/iio/imu/bmi160/bmi160.h      | 2 ++
 drivers/iio/imu/bmi160/bmi160_core.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index 621f5309d735..fa071d9230ec 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -7,6 +7,8 @@
 struct bmi160_data {
 	struct regmap *regmap;
 	struct iio_trigger *trig;
+	/* Ensure natural alignment for timestamp if present */
+	__le16 buf[16] __aligned(8);
 };
 
 extern const struct regmap_config bmi160_regmap_config;
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index 6af65d6f1d28..81977d427687 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -411,7 +411,6 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct bmi160_data *data = iio_priv(indio_dev);
-	__le16 buf[16];
 	/* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */
 	int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
 	__le16 sample;
@@ -422,10 +421,10 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
 				       &sample, sizeof(sample));
 		if (ret)
 			goto done;
-		buf[j++] = sample;
+		data->buf[j++] = sample;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buf, pf->timestamp);
 done:
 	iio_trigger_notify_done(indio_dev->trig);
 	return IRQ_HANDLED;
-- 
2.26.2

[PATCH 12/25] iio:imu:st_lsm6dsx Fix alignment and data leak issues

[Jonathan Cameron]

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;
 			}
 			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++;
-- 
2.26.2

[PATCH 13/25] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This case is a bit different to the rest of the series.  The driver
was doing a regmap_bulk_read into a buffer that wasn't dma safe
as it was on the stack with no guarantee of it being in a cacheline
on it's own.   Fixing that also dealt with the data leak and
alignment issues that Lars-Peter pointed out.

Also removed some unaligned handling as we are now aligned.

Fixes tag is for the dma safe buffer issue. Potentially we would
need to backport timestamp alignment futher but that is a totally
different patch.

Fixes: fd64df16f40e ("iio: imu: inv_mpu6050: Add SPI support for MPU6000")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 +++++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 12 ++++++------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index cd38b3fccc7b..e4df2d51b689 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -122,6 +122,9 @@ struct inv_mpu6050_chip_config {
 	u8 user_ctrl;
 };
 
+/* 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8 */
+#define INV_MPU6050_OUTPUT_DATA_SIZE         32
+
 /**
  *  struct inv_mpu6050_hw - Other important hardware information.
  *  @whoami:	Self identification byte from WHO_AM_I register
@@ -165,6 +168,7 @@ struct inv_mpu6050_hw {
  *  @magn_raw_to_gauss:	coefficient to convert mag raw value to Gauss.
  *  @magn_orient:       magnetometer sensor chip orientation if available.
  *  @suspended_sensors:	sensors mask of sensors turned off for suspend
+ *  @data:		dma safe buffer used for bulk reads.
  */
 struct inv_mpu6050_state {
 	struct mutex lock;
@@ -190,6 +194,7 @@ struct inv_mpu6050_state {
 	s32 magn_raw_to_gauss[3];
 	struct iio_mount_matrix magn_orient;
 	unsigned int suspended_sensors;
+	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE] ____cacheline_aligned;
 };
 
 /*register and associated bit definition*/
@@ -334,9 +339,6 @@ struct inv_mpu6050_state {
 #define INV_ICM20608_TEMP_OFFSET	     8170
 #define INV_ICM20608_TEMP_SCALE		     3059976
 
-/* 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8 */
-#define INV_MPU6050_OUTPUT_DATA_SIZE         32
-
 #define INV_MPU6050_REG_INT_PIN_CFG	0x37
 #define INV_MPU6050_ACTIVE_HIGH		0x00
 #define INV_MPU6050_ACTIVE_LOW		0x80
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 9511e4715e2c..554c16592d47 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -13,7 +13,6 @@
 #include <linux/interrupt.h>
 #include <linux/poll.h>
 #include <linux/math64.h>
-#include <asm/unaligned.h>
 #include "inv_mpu_iio.h"
 
 /**
@@ -121,7 +120,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	size_t bytes_per_datum;
 	int result;
-	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
 	u16 fifo_count;
 	s64 timestamp;
 	int int_status;
@@ -160,11 +158,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	 * read fifo_count register to know how many bytes are inside the FIFO
 	 * right now
 	 */
-	result = regmap_bulk_read(st->map, st->reg->fifo_count_h, data,
+	result = regmap_bulk_read(st->map, st->reg->fifo_count_h,
+				  st->data,
 				  INV_MPU6050_FIFO_COUNT_BYTE);
 	if (result)
 		goto end_session;
-	fifo_count = get_unaligned_be16(&data[0]);
+	fifo_count = be16_to_cpup((__be16 *)&st->data[0]);
 
 	/*
 	 * Handle fifo overflow by resetting fifo.
@@ -182,7 +181,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	inv_mpu6050_update_period(st, pf->timestamp, nb);
 	for (i = 0; i < nb; ++i) {
 		result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
-					  data, bytes_per_datum);
+					  st->data, bytes_per_datum);
 		if (result)
 			goto flush_fifo;
 		/* skip first samples if needed */
@@ -191,7 +190,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 			continue;
 		}
 		timestamp = inv_mpu6050_get_timestamp(st);
-		iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
+		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
+						   timestamp);
 	}
 
 end_session:
-- 
2.26.2

[PATCH 14/25] iio:pressure:ms5611 Fix buffer element alignment

[Jonathan Cameron]

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.
Here there is no data leak possibility so use an explicit structure
on the stack to ensure alignment and nice readable fashion.

Fixes: 713bbb4efb9dc ("iio: pressure: ms5611: Add triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/pressure/ms5611_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
index 2f598ad91621..ba9cf9d77a4a 100644
--- a/drivers/iio/pressure/ms5611_core.c
+++ b/drivers/iio/pressure/ms5611_core.c
@@ -212,16 +212,21 @@ static irqreturn_t ms5611_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ms5611_state *st = iio_priv(indio_dev);
-	s32 buf[4]; /* s32 (pressure) + s32 (temp) + 2 * s32 (timestamp) */
+	/* Ensure buffer elements are naturally aligned */
+	struct {
+		s32 channels[2];
+		s64 ts;
+	} scan;
 	int ret;
 
 	mutex_lock(&st->lock);
-	ret = ms5611_read_temp_and_pressure(indio_dev, &buf[1], &buf[0]);
+	ret = ms5611_read_temp_and_pressure(indio_dev, &scan.channels[1],
+					    &scan.channels[0]);
 	mutex_unlock(&st->lock);
 	if (ret < 0)
 		goto err;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
 					   iio_get_time_ns(indio_dev));
 
 err:
-- 
2.26.2

[PATCH 15/25] iio:pressure:mpl3115 Force alignment of buffer

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Whilst this is another case of the issue Lars reported with
an array of elements of smaller than 8 bytes being passed
to iio_push_to_buffers_with_timestamp, the solution here is
a bit different from the other cases and relies on __aligned
working on the stack (true since 4.6?)

This one is unusual.  We have to do an explicit memset each time
as we are reading 3 bytes into a potential 4 byte channel which
may sometimes be a 2 byte channel depending on what is enabled.
As such, moving the buffer to the heap in the iio_priv structure
doesn't save us much.  We can't use a nice explicit structure
on the stack either as the data channels have different storage
sizes and are all separately controlled.

Fixes: cc26ad455f57 ("iio: Add Freescale MPL3115A2 pressure / temperature sensor driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/pressure/mpl3115.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index d066f3c5a8a6..1eb761befcbe 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -144,7 +144,8 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mpl3115_data *data = iio_priv(indio_dev);
-	u8 buffer[16]; /* 32-bit channel + 16-bit channel + padding + ts */
+	/* 32-bit channel + 16-bit channel + padding + ts */
+	u8 buffer[16] __aligned(8);
 	int ret, pos = 0;
 
 	mutex_lock(&data->lock);
-- 
2.26.2

[PATCH 16/25] iio:adc:ti-adc081c Fix alignment and data leak issues

[Jonathan Cameron]

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().

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

Fixes: 08e05d1fce5c (" ti-adc081c: Initial triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-adc081c.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
index 0235863ff77b..a5c1a438370d 100644
--- a/drivers/iio/adc/ti-adc081c.c
+++ b/drivers/iio/adc/ti-adc081c.c
@@ -33,6 +33,12 @@ struct adc081c {
 
 	/* 8, 10 or 12 */
 	int bits;
+
+	/* Ensure natural alignment of buffer elements */
+	struct {
+		u16 channel;
+		s64 ts;
+	} scan;
 };
 
 #define REG_CONV_RES 0x00
@@ -128,14 +134,13 @@ static irqreturn_t adc081c_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adc081c *data = iio_priv(indio_dev);
-	u16 buf[8]; /* 2 bytes data + 6 bytes padding + 8 bytes timestamp */
 	int ret;
 
 	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
 	if (ret < 0)
 		goto out;
-	buf[0] = ret;
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	data->scan.channel = ret;
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
 					   iio_get_time_ns(indio_dev));
 out:
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.26.2

[PATCH 17/25] iio:adc:ti-adc084s021 Fix alignment and data leak issues.

[Jonathan Cameron]

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().

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

Fixes: 3691e5a69449 ("iio: adc: add driver for the ti-adc084s021 chip")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mårten Lindahl <martenli@axis.com>
---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
index bdedf456ee05..36d874cced9d 100644
--- a/drivers/iio/adc/ti-adc084s021.c
+++ b/drivers/iio/adc/ti-adc084s021.c
@@ -25,6 +25,11 @@ struct adc084s021 {
 	struct spi_transfer spi_trans;
 	struct regulator *reg;
 	struct mutex lock;
+	/* Buffer used to align data */
+	struct {
+		__be16 channels[4];
+		s64 ts;
+	} scan;
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache line.
@@ -140,14 +145,13 @@ static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void *pollfunc)
 	struct iio_poll_func *pf = pollfunc;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adc084s021 *adc = iio_priv(indio_dev);
-	__be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes timestamp */
 
 	mutex_lock(&adc->lock);
 
-	if (adc084s021_adc_conversion(adc, &data) < 0)
+	if (adc084s021_adc_conversion(adc, adc->scan.channels) < 0)
 		dev_err(&adc->spi->dev, "Failed to read data\n");
 
-	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));
 	mutex_unlock(&adc->lock);
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.26.2

[PATCH 18/25] iio:adc:ti-adc084s021 Tidy up endian types

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

By adding a few local variables and avoiding a void * for
a parameter we can easily make all the endian types explicit and
get rid of the warnings from sparse:

  CHECK   drivers/iio/adc/ti-adc084s021.c
drivers/iio/adc/ti-adc084s021.c:84:26: warning: incorrect type in assignment (different base types)
drivers/iio/adc/ti-adc084s021.c:84:26:    expected unsigned short [usertype]
drivers/iio/adc/ti-adc084s021.c:84:26:    got restricted __be16
drivers/iio/adc/ti-adc084s021.c:115:24: warning: cast to restricted __be16
drivers/iio/adc/ti-adc084s021.c:115:24: warning: cast to restricted __be16
drivers/iio/adc/ti-adc084s021.c:115:24: warning: cast to restricted __be16
drivers/iio/adc/ti-adc084s021.c:115:24: warning: cast to restricted __be16

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
index 36d874cced9d..4e393cb44caa 100644
--- a/drivers/iio/adc/ti-adc084s021.c
+++ b/drivers/iio/adc/ti-adc084s021.c
@@ -69,11 +69,10 @@ static const struct iio_chan_spec adc084s021_channels[] = {
  * @adc: The ADC SPI data.
  * @data: Buffer for converted data.
  */
-static int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)
+static int adc084s021_adc_conversion(struct adc084s021 *adc, __be16 *data)
 {
 	int n_words = (adc->spi_trans.len >> 1) - 1; /* Discard first word */
 	int ret, i = 0;
-	u16 *p = data;
 
 	/* Do the transfer */
 	ret = spi_sync(adc->spi, &adc->message);
@@ -81,7 +80,7 @@ static int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)
 		return ret;
 
 	for (; i < n_words; i++)
-		*(p + i) = adc->rx_buf[i + 1];
+		*(data + i) = adc->rx_buf[i + 1];
 
 	return ret;
 }
@@ -92,6 +91,7 @@ static int adc084s021_read_raw(struct iio_dev *indio_dev,
 {
 	struct adc084s021 *adc = iio_priv(indio_dev);
 	int ret;
+	__be16 be_val;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -106,13 +106,13 @@ static int adc084s021_read_raw(struct iio_dev *indio_dev,
 		}
 
 		adc->tx_buf[0] = channel->channel << 3;
-		ret = adc084s021_adc_conversion(adc, val);
+		ret = adc084s021_adc_conversion(adc, &be_val);
 		iio_device_release_direct_mode(indio_dev);
 		regulator_disable(adc->reg);
 		if (ret < 0)
 			return ret;
 
-		*val = be16_to_cpu(*val);
+		*val = be16_to_cpu(be_val);
 		*val = (*val >> channel->scan_type.shift) & 0xff;
 
 		return IIO_VAL_INT;
-- 
2.26.2

[PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Jonathan Cameron]

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.

Here we use an explicit structure and rely on that to enforce
alignment on the stack.  Note there was never a data leak here
due to the explicit memset.

Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/ti-ads1015.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 5ea4f45d6bad..05853723dbdb 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ads1015_data *data = iio_priv(indio_dev);
-	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
+	/* Ensure natural alignment for buffer elements */
+	struct {
+		s16 channel;
+		s64 ts;
+	} scan;
 	int chan, ret, res;
 
-	memset(buf, 0, sizeof(buf));
+	memset(&scan, 0, sizeof(scan));
 
 	mutex_lock(&data->lock);
 	chan = find_first_bit(indio_dev->active_scan_mask,
@@ -399,10 +403,10 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
 		goto err;
 	}
 
-	buf[0] = res;
+	scan.channel = res;
 	mutex_unlock(&data->lock);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
 					   iio_get_time_ns(indio_dev));
 
 err:
-- 
2.26.2

[PATCH 20/25] iio:adc:ti-ads124s08 Fix alignment and data leak issues.

[Jonathan Cameron]

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 with alignment
explicitly requested.  This data is allocated with kzalloc so no
data can leak apart from previous readings.

Fixes: e717f8c6dfec ("iio: adc: Add the TI ads124s08 ADC code")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/iio/adc/ti-ads124s08.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
index f1ee3b1e2827..fb0da62b09f5 100644
--- a/drivers/iio/adc/ti-ads124s08.c
+++ b/drivers/iio/adc/ti-ads124s08.c
@@ -99,6 +99,11 @@ struct ads124s_private {
 	struct gpio_desc *reset_gpio;
 	struct spi_device *spi;
 	struct mutex lock;
+	/*
+	 * Used to correctly align data.
+	 * Ensure timestamp is naturally aligned.
+	 */
+	u32 buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);
 	u8 data[5] ____cacheline_aligned;
 };
 
@@ -269,7 +274,6 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ads124s_private *priv = iio_priv(indio_dev);
-	u32 buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u16)];
 	int scan_index, j = 0;
 	int ret;
 
@@ -284,7 +288,7 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)
 		if (ret)
 			dev_err(&priv->spi->dev, "Start ADC conversions failed\n");
 
-		buffer[j] = ads124s_read(indio_dev, scan_index);
+		priv->buffer[j] = ads124s_read(indio_dev, scan_index);
 		ret = ads124s_write_cmd(indio_dev, ADS124S08_STOP_CONV);
 		if (ret)
 			dev_err(&priv->spi->dev, "Stop ADC conversions failed\n");
@@ -292,7 +296,7 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)
 		j++;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+	iio_push_to_buffers_with_timestamp(indio_dev, priv->buffer,
 			pf->timestamp);
 
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.26.2

[PATCH 21/25] iio:adc:ti-ads8688 Fix alignment and potential data leak issue

[Jonathan Cameron]

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 32 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
explicitly requested.  This data is allocated with kzalloc so no
data can leak apart from previous readings.

Fixes: 2a86487786b5 ("iio: adc: ti-ads8688: add trigger and buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/adc/ti-ads8688.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
index 14fe7c320b52..8acc0f59de60 100644
--- a/drivers/iio/adc/ti-ads8688.c
+++ b/drivers/iio/adc/ti-ads8688.c
@@ -68,6 +68,12 @@ struct ads8688_state {
 	struct regulator		*reg;
 	unsigned int			vref_mv;
 	enum ads8688_range		range[8];
+	/*
+	 * Used to align data for pushing to IIO.
+	 * Ensure natural alignment of timestamps
+	 */
+	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);
+
 	union {
 		__be32 d32;
 		u8 d8[4];
@@ -383,17 +389,17 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
-	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)];
+	struct ads8688_state *st = iio_priv(indio_dev);
 	int i, j = 0;
 
 	for (i = 0; i < indio_dev->masklength; i++) {
 		if (!test_bit(i, indio_dev->active_scan_mask))
 			continue;
-		buffer[j] = ads8688_read(indio_dev, i);
+		st->buffer[j] = ads8688_read(indio_dev, i);
 		j++;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
 			iio_get_time_ns(indio_dev));
 
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.26.2

[PATCH 22/25] iio:adc:ti-adc0832 Fix alignment issue with timestamp

[Jonathan Cameron]

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.

We fix this issues by moving to a suitable structure in the iio_priv()
data with alignment explicitly requested.  This data is allocated
with kzalloc so no data can leak apart from previous readings.
Note that previously no data could leak 'including' previous readings
but I don't think it is an issue to potentially leak them like
this now does.

Fixes: 815bbc87462a ("iio: ti-adc0832: add triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-adc0832.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-adc0832.c b/drivers/iio/adc/ti-adc0832.c
index 6ea39f4bbb37..f94c862b7821 100644
--- a/drivers/iio/adc/ti-adc0832.c
+++ b/drivers/iio/adc/ti-adc0832.c
@@ -28,6 +28,8 @@ struct adc0832 {
 	struct regulator *reg;
 	struct mutex lock;
 	u8 mux_bits;
+	/* 16x 1 byte ADC data + 8 bytes timestamp */
+	u8 data[24] __aligned(8);
 
 	u8 tx_buf[2] ____cacheline_aligned;
 	u8 rx_buf[2];
@@ -199,7 +201,6 @@ static irqreturn_t adc0832_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adc0832 *adc = iio_priv(indio_dev);
-	u8 data[24] = { }; /* 16x 1 byte ADC data + 8 bytes timestamp */
 	int scan_index;
 	int i = 0;
 
@@ -217,10 +218,10 @@ static irqreturn_t adc0832_trigger_handler(int irq, void *p)
 			goto out;
 		}
 
-		data[i] = ret;
+		adc->data[i] = ret;
 		i++;
 	}
-	iio_push_to_buffers_with_timestamp(indio_dev, data,
+	iio_push_to_buffers_with_timestamp(indio_dev, adc->data,
 					   iio_get_time_ns(indio_dev));
 out:
 	mutex_unlock(&adc->lock);
-- 
2.26.2

[PATCH 23/25] iio:adc:ti-adc12138 Fix alignment issue with timestamp

[Jonathan Cameron]

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.

We move to a suitable structure in the iio_priv() data with alignment
explicitly requested.  This data is allocated with kzalloc so no
data can leak apart from previous readings. Note that previously
no leak at all could occur, but previous readings should never
be a problem.

Fixes: 50a6edb1b6e0 ("iio: adc: add ADC12130/ADC12132/ADC12138 ADC driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-adc12138.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-adc12138.c b/drivers/iio/adc/ti-adc12138.c
index 68a9dcb8faa2..f764c3694a96 100644
--- a/drivers/iio/adc/ti-adc12138.c
+++ b/drivers/iio/adc/ti-adc12138.c
@@ -47,6 +47,8 @@ struct adc12138 {
 	struct completion complete;
 	/* The number of cclk periods for the S/H's acquisition time */
 	unsigned int acquisition_time;
+	 /* 16x 2 bytes ADC data + 8 bytes timestamp */
+	__be16 data[20] __aligned(8);
 
 	u8 tx_buf[2] ____cacheline_aligned;
 	u8 rx_buf[2];
@@ -329,7 +331,6 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adc12138 *adc = iio_priv(indio_dev);
-	__be16 data[20] = { }; /* 16x 2 bytes ADC data + 8 bytes timestamp */
 	__be16 trash;
 	int ret;
 	int scan_index;
@@ -345,7 +346,7 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
 		reinit_completion(&adc->complete);
 
 		ret = adc12138_start_and_read_conv(adc, scan_chan,
-						   i ? &data[i - 1] : &trash);
+					i ? &adc->data[i - 1] : &trash);
 		if (ret) {
 			dev_warn(&adc->spi->dev,
 				 "failed to start conversion\n");
@@ -362,7 +363,7 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
 	}
 
 	if (i) {
-		ret = adc12138_read_conv_data(adc, &data[i - 1]);
+		ret = adc12138_read_conv_data(adc, &adc->data[i - 1]);
 		if (ret) {
 			dev_warn(&adc->spi->dev,
 				 "failed to get conversion data\n");
@@ -370,7 +371,7 @@ static irqreturn_t adc12138_trigger_handler(int irq, void *p)
 		}
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data,
+	iio_push_to_buffers_with_timestamp(indio_dev, adc->data,
 					   iio_get_time_ns(indio_dev));
 out:
 	mutex_unlock(&adc->lock);
-- 
2.26.2

[PATCH 24/25] iio:adc:ina2xx Fix timestamp alignment issue.

[Jonathan Cameron]

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 32 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
explicitly requested.  This data is allocated with kzalloc so no
data can leak apart from previous readings.

If we want this in older stables will need manual backport due to
driver reworks.

Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Cc: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/ina2xx-adc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index bdd7cba6f6b0..ed2d069b791a 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -146,6 +146,8 @@ struct ina2xx_chip_info {
 	int range_vbus; /* Bus voltage maximum in V */
 	int pga_gain_vshunt; /* Shunt voltage PGA gain */
 	bool allow_async_readout;
+	/* data buffer needs space for channel data and timestap */
+	unsigned short data[4 + sizeof(s64)/sizeof(short)] __aligned(8);
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -738,8 +740,6 @@ static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
 static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 {
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
-	/* data buffer needs space for channel data and timestap */
-	unsigned short data[4 + sizeof(s64)/sizeof(short)];
 	int bit, ret, i = 0;
 	s64 time;
 
@@ -758,10 +758,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 		if (ret < 0)
 			return ret;
 
-		data[i++] = val;
+		chip->data[i++] = val;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
+	iio_push_to_buffers_with_timestamp(indio_dev, chip->data, time);
 
 	return 0;
 };
-- 
2.26.2

[PATCH 25/25] iio:adc:max1118 Fix alignment of timestamp and data leak issues

[Jonathan Cameron]

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.

Fixes: a9e9c7153e96 ("iio: adc: add max1117/max1118/max1119 ADC driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
---
 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 0c5d7aaf6826..32296cc6fa9b 100644
--- a/drivers/iio/adc/max1118.c
+++ b/drivers/iio/adc/max1118.c
@@ -35,6 +35,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;
+	} scan;
 
 	u8 data ____cacheline_aligned;
 };
@@ -165,7 +170,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;
 
@@ -183,10 +187,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);
-- 
2.26.2

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko]

On Mon, May 25, 2020 at 06:06:22PM +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 an array of smaller elements on the stack.
> 
> Here we use an explicit structure and rely on that to enforce
> alignment on the stack.  Note there was never a data leak here
> due to the explicit memset.
> 
> Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/adc/ti-ads1015.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 5ea4f45d6bad..05853723dbdb 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ads1015_data *data = iio_priv(indio_dev);
> -	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
> +	/* Ensure natural alignment for buffer elements */
> +	struct {
> +		s16 channel;
> +		s64 ts;
> +	} scan;

Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
iio_push_to_buffers_with_timestamp() point of view?

>  	int chan, ret, res;
>  
> -	memset(buf, 0, sizeof(buf));
> +	memset(&scan, 0, sizeof(scan));
>  
>  	mutex_lock(&data->lock);
>  	chan = find_first_bit(indio_dev->active_scan_mask,
> @@ -399,10 +403,10 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>  		goto err;
>  	}
>  
> -	buf[0] = res;
> +	scan.channel = res;
>  	mutex_unlock(&data->lock);
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
>  					   iio_get_time_ns(indio_dev));
>  
>  err:
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/25] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.

[Jean-Baptiste Maneyrol]

Hi Jonathan,

the driver is also doing other regmap_bulk_read and regmap_bulk_write for getting sensor data from registers and for setting hardware offsets.
But there are numerous IIO drivers supporting SPI that are doing the same (IMU ST driver for example).

Are you sure this is really required? It seems to just call standard SPI transfers behind (I don't know if DMA can be implicitly call there). This is very inconvenient to deal with if there is no easy way to read/write multiple registers on the stack.

Another thing I have in mind, the read for the FIFO should be replaced by a regmap_noinc_read, since this is a virtual register reading the FIFO, not the register map.

Thanks,
JB

From: Jonathan Cameron <jic23@kernel.org>
Sent: Monday, May 25, 2020 19:06
To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
Subject: [PATCH 13/25] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues. 
 
 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This case is a bit different to the rest of the series.  The driver
was doing a regmap_bulk_read into a buffer that wasn't dma safe
as it was on the stack with no guarantee of it being in a cacheline
on it's own.   Fixing that also dealt with the data leak and
alignment issues that Lars-Peter pointed out.

Also removed some unaligned handling as we are now aligned.

Fixes tag is for the dma safe buffer issue. Potentially we would
need to backport timestamp alignment futher but that is a totally
different patch.

Fixes: fd64df16f40e ("iio: imu: inv_mpu6050: Add SPI support for MPU6000")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 +++++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 12 ++++++------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index cd38b3fccc7b..e4df2d51b689 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -122,6 +122,9 @@ struct inv_mpu6050_chip_config {
         u8 user_ctrl;
 };
 
+/* 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8 */
+#define INV_MPU6050_OUTPUT_DATA_SIZE         32
+
 /**
  *  struct inv_mpu6050_hw - Other important hardware information.
  *  @whoami:   Self identification byte from WHO_AM_I register
@@ -165,6 +168,7 @@ struct inv_mpu6050_hw {
  *  @magn_raw_to_gauss:        coefficient to convert mag raw value to Gauss.
  *  @magn_orient:       magnetometer sensor chip orientation if available.
  *  @suspended_sensors:        sensors mask of sensors turned off for suspend
+ *  @data:             dma safe buffer used for bulk reads.
  */
 struct inv_mpu6050_state {
         struct mutex lock;
@@ -190,6 +194,7 @@ struct inv_mpu6050_state {
         s32 magn_raw_to_gauss[3];
         struct iio_mount_matrix magn_orient;
         unsigned int suspended_sensors;
+       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE] ____cacheline_aligned;
 };
 
 /*register and associated bit definition*/
@@ -334,9 +339,6 @@ struct inv_mpu6050_state {
 #define INV_ICM20608_TEMP_OFFSET             8170
 #define INV_ICM20608_TEMP_SCALE              3059976
 
-/* 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8 */
-#define INV_MPU6050_OUTPUT_DATA_SIZE         32
-
 #define INV_MPU6050_REG_INT_PIN_CFG     0x37
 #define INV_MPU6050_ACTIVE_HIGH         0x00
 #define INV_MPU6050_ACTIVE_LOW          0x80
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 9511e4715e2c..554c16592d47 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -13,7 +13,6 @@
 #include <linux/interrupt.h>
 #include <linux/poll.h>
 #include <linux/math64.h>
-#include <asm/unaligned.h>
 #include "inv_mpu_iio.h"
 
 /**
@@ -121,7 +120,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
         struct inv_mpu6050_state *st = iio_priv(indio_dev);
         size_t bytes_per_datum;
         int result;
-       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
         u16 fifo_count;
         s64 timestamp;
         int int_status;
@@ -160,11 +158,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
          * read fifo_count register to know how many bytes are inside the FIFO
          * right now
          */
-       result = regmap_bulk_read(st->map, st->reg->fifo_count_h, data,
+       result = regmap_bulk_read(st->map, st->reg->fifo_count_h,
+                                 st->data,
                                   INV_MPU6050_FIFO_COUNT_BYTE);
         if (result)
                 goto end_session;
-       fifo_count = get_unaligned_be16(&data[0]);
+       fifo_count = be16_to_cpup((__be16 *)&st->data[0]);
 
         /*
          * Handle fifo overflow by resetting fifo.
@@ -182,7 +181,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
         inv_mpu6050_update_period(st, pf->timestamp, nb);
         for (i = 0; i < nb; ++i) {
                 result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
-                                         data, bytes_per_datum);
+                                         st->data, bytes_per_datum);
                 if (result)
                         goto flush_fifo;
                 /* skip first samples if needed */
@@ -191,7 +190,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
                         continue;
                 }
                 timestamp = inv_mpu6050_get_timestamp(st);
-               iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
+               iio_push_to_buffers_with_timestamp(indio_dev, st->data,
+                                                  timestamp);
         }
 
 end_session:
-- 
2.26.2

Re: [PATCH 10/25] iio:humidity:hts221 Fix alignment and data leak issues

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3134 bytes --]

> 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.
> 
> Fixes: e4a70e3e7d84 ("iio: humidity: add support to hts221 rh/temp combo device")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>

Hi Jonathan,

I guess you can drop HTS221_DATA_SIZE now since it seems no longer used.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  drivers/iio/humidity/hts221.h        | 5 +++++
>  drivers/iio/humidity/hts221_buffer.c | 9 +++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
> index 7c650df77556..6ad579ca9cef 100644
> --- a/drivers/iio/humidity/hts221.h
> +++ b/drivers/iio/humidity/hts221.h
> @@ -39,6 +39,11 @@ struct hts221_hw {
>  
>  	bool enabled;
>  	u8 odr;
> +	/* Ensure natural alignment of timestamp */
> +	struct {
> +		__le16 channels[2];
> +		s64 ts;
> +	} scan;
>  };
>  
>  extern const struct dev_pm_ops hts221_pm_ops;
> diff --git a/drivers/iio/humidity/hts221_buffer.c b/drivers/iio/humidity/hts221_buffer.c
> index 21c6c160462d..59ede9860185 100644
> --- a/drivers/iio/humidity/hts221_buffer.c
> +++ b/drivers/iio/humidity/hts221_buffer.c
> @@ -160,7 +160,6 @@ static const struct iio_buffer_setup_ops hts221_buffer_ops = {
>  
>  static irqreturn_t hts221_buffer_handler_thread(int irq, void *p)
>  {
> -	u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)];
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *iio_dev = pf->indio_dev;
>  	struct hts221_hw *hw = iio_priv(iio_dev);
> @@ -170,18 +169,20 @@ static irqreturn_t hts221_buffer_handler_thread(int irq, void *p)
>  	/* humidity data */
>  	ch = &iio_dev->channels[HTS221_SENSOR_H];
>  	err = regmap_bulk_read(hw->regmap, ch->address,
> -			       buffer, HTS221_DATA_SIZE);
> +			       &hw->scan.channels[0],
> +			       sizeof(hw->scan.channels[0]));
>  	if (err < 0)
>  		goto out;
>  
>  	/* temperature data */
>  	ch = &iio_dev->channels[HTS221_SENSOR_T];
>  	err = regmap_bulk_read(hw->regmap, ch->address,
> -			       buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE);
> +			       &hw->scan.channels[1],
> +			       sizeof(hw->scan.channels[1]));
>  	if (err < 0)
>  		goto out;
>  
> -	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
> +	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
>  					   iio_get_time_ns(iio_dev));
>  
>  out:
> -- 
> 2.26.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/25] iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

> 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()
> 
> This data is allocated with kzalloc so no data can leak apart
> from previous readings.
> 
> Fixes: 3025c8688c1e ("iio: light: add support for UVIS25 sensor")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  drivers/iio/light/st_uvis25.h      | 5 +++++
>  drivers/iio/light/st_uvis25_core.c | 6 +++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/st_uvis25.h b/drivers/iio/light/st_uvis25.h
> index 78bc56aad129..f7027e4c4493 100644
> --- a/drivers/iio/light/st_uvis25.h
> +++ b/drivers/iio/light/st_uvis25.h
> @@ -27,6 +27,11 @@ struct st_uvis25_hw {
>  	struct iio_trigger *trig;
>  	bool enabled;
>  	int irq;
> +	/* Ensure timestamp is naturally aligned */
> +	struct {
> +		u8 chan;
> +		s64 ts;
> +	} scan;
>  };
>  
>  extern const struct dev_pm_ops st_uvis25_pm_ops;
> diff --git a/drivers/iio/light/st_uvis25_core.c b/drivers/iio/light/st_uvis25_core.c
> index d262c254b895..fe1f2dc970c7 100644
> --- a/drivers/iio/light/st_uvis25_core.c
> +++ b/drivers/iio/light/st_uvis25_core.c
> @@ -234,17 +234,17 @@ static const struct iio_buffer_setup_ops st_uvis25_buffer_ops = {
>  
>  static irqreturn_t st_uvis25_buffer_handler_thread(int irq, void *p)
>  {
> -	u8 buffer[ALIGN(sizeof(u8), sizeof(s64)) + sizeof(s64)];
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *iio_dev = pf->indio_dev;
>  	struct st_uvis25_hw *hw = iio_priv(iio_dev);
>  	int err;
>  
> -	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR, (int *)buffer);
> +	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR,
> +			  (unsigned int *)&hw->scan.chan);
>  	if (err < 0)
>  		goto out;
>  
> -	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
> +	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
>  					   iio_get_time_ns(iio_dev));
>  
>  out:
> -- 
> 2.26.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 12/25] iio:imu:st_lsm6dsx Fix alignment and data leak issues

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4766 bytes --]

> 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Lars-Peter Clausen]

On 5/25/20 7:52 PM, Andy Shevchenko wrote:
> On Mon, May 25, 2020 at 06:06:22PM +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 an array of smaller elements on the stack.
>>
>> Here we use an explicit structure and rely on that to enforce
>> alignment on the stack.  Note there was never a data leak here
>> due to the explicit memset.
>>
>> Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support")
>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/iio/adc/ti-ads1015.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
>> index 5ea4f45d6bad..05853723dbdb 100644
>> --- a/drivers/iio/adc/ti-ads1015.c
>> +++ b/drivers/iio/adc/ti-ads1015.c
>> @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>>   	struct iio_poll_func *pf = p;
>>   	struct iio_dev *indio_dev = pf->indio_dev;
>>   	struct ads1015_data *data = iio_priv(indio_dev);
>> -	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
>> +	/* Ensure natural alignment for buffer elements */
>> +	struct {
>> +		s16 channel;
>> +		s64 ts;
>> +	} scan;
> Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
> iio_push_to_buffers_with_timestamp() point of view?

No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. 
Looks like we can't rely on implicit padding, but need to always 
explicitly specify it.

Or maybe we can typedef and IIO timestamp type with an explicit 
__aligned attribute. I wonder if that works... After having a quick 
look, the kernel already defines aligned_u64, so maybe using that is an 
option.

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko]

On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote:
> On 5/25/20 7:52 PM, Andy Shevchenko wrote:
> > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote:
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

...

> > > -	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
> > > +	/* Ensure natural alignment for buffer elements */
> > > +	struct {
> > > +		s16 channel;
> > > +		s64 ts;
> > > +	} scan;
> > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
> > iio_push_to_buffers_with_timestamp() point of view?
> 
> No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks
> like we can't rely on implicit padding, but need to always explicitly
> specify it.
> 
> Or maybe we can typedef and IIO timestamp type with an explicit __aligned
> attribute. I wonder if that works... After having a quick look, the kernel
> already defines aligned_u64, so maybe using that is an option.

Another way is simple to provide offset of timestamp member as a parameter.
Though, if it's an ABI, then alas, we need to align it properly.

Also, wouldn't be better to explicitly show the padding?

	struct {
		s16 channel;
		s16 padding[3];
		s64 ts;
	} scan;

(matter of style though, just saying).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 07/25] iio:magnetometer:ak8975 Fix alignment and data leak issues.

[Andy Shevchenko]

On Mon, May 25, 2020 at 06:06:10PM +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 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.

I'm not sure I understood the issue in full.

s16 members are aligned on 2 bytes at least. Unaligned access easily to fix by
moving from (int64_t *)... = ...; to put_unaligned(); in
iio_push_to_buffers_with_timestamp() once for all.

On stack the driver allocates proper amount of data with padding.

> This data is allocated with kzalloc so no data can leak apart from
> previous readings.
> 
> Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support")

Unfortunately breaks as per other patch review :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/25] iio:magnetometer:ak8974: Fix alignment and data leak issues

[Linus Walleij]

On Mon, May 25, 2020 at 7:09 PM 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 appart from
> previous readings.
>
> Fixes: 7c94a8b2ee8cf ("iio: magn: add a driver for AK8974")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Whoa, good catch! Definitely my mindless coding.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Jonathan Cameron]

On Tue, 26 May 2020 12:15:56 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote:
> > On 5/25/20 7:52 PM, Andy Shevchenko wrote:  
> > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote:  
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> ...
> 
> > > > -	s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding +  4x s16 timestamp */
> > > > +	/* Ensure natural alignment for buffer elements */
> > > > +	struct {
> > > > +		s16 channel;
> > > > +		s64 ts;
> > > > +	} scan;  
> > > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
> > > iio_push_to_buffers_with_timestamp() point of view?  
> > 
> > No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks
> > like we can't rely on implicit padding, but need to always explicitly
> > specify it.
> > 
> > Or maybe we can typedef and IIO timestamp type with an explicit __aligned
> > attribute. I wonder if that works... After having a quick look, the kernel
> > already defines aligned_u64, so maybe using that is an option.  
> 
> Another way is simple to provide offset of timestamp member as a parameter.
> Though, if it's an ABI, then alas, we need to align it properly.
> 
> Also, wouldn't be better to explicitly show the padding?
> 
> 	struct {
> 		s16 channel;
> 		s16 padding[3];
> 		s64 ts;
> 	} scan;
> 
> (matter of style though, just saying).
> 

gah.  Thanks for pointing this out Andy.  I wanted to avoid explicitly
calling out empty padding because it seemed to me to be more likely to
be error prone than filling it in.

I was trying to avoid using __aligned on the stack as it only works for
more recent kernels (due to gcc version changes) and some of these predate
that point.

I guess we just do it explicitly in all these cases.

The two patches that have already gone to Greg both have sufficient
data to ensure the structure is big enough (only 16 bytes padding in one and
none in the other).

I think we are also fine for the original question as well as it won't
matter if the whole structure is aligned to 4 bytes on x86_32 and
similar as an 8 byte write will be fine.

So fun question - do we want to enforce 8 byte alignment of the whole
structure, or simply the padding?

Maybe better to just do the padding explicitly as Andy suggested.

Jonathan

Re: [PATCH 07/25] iio:magnetometer:ak8975 Fix alignment and data leak issues.

[Jonathan Cameron]

On Tue, 26 May 2020 12:24:15 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, May 25, 2020 at 06:06:10PM +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 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.  
> 
> I'm not sure I understood the issue in full.
> 
> s16 members are aligned on 2 bytes at least. Unaligned access easily to fix by
> moving from (int64_t *)... = ...; to put_unaligned(); in
> iio_push_to_buffers_with_timestamp() once for all.

The problem is that consumers of the buffer also need to know
that it's potentially unaligned.  So ultimately all consumers need
to then do a get_unaligned for the timestamp.

Note in some of these cases they would need to do a get_unaligned
for the channels as well.  So I think we are better off fixing all
these up and ensuring predictability. 

Moving to the heap here was just to avoid having to memset the thing
each time rather than the alignment issue.

> 
> On stack the driver allocates proper amount of data with padding.
> 
> > This data is allocated with kzalloc so no data can leak apart from
> > previous readings.
> > 
> > Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support")  
> 
> Unfortunately breaks as per other patch review :-)
> 

Actually I think this one is coincidentally fine as we have 6 bytes of channels,
but see that other thread for why. + we'd still want to change the
code here to make that explicit.

Jonathan

Re: [PATCH 04/25] iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Tue, 26 May 2020 09:55:05 +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 suitable structure in the iio_priv()
> > 
> > This data is allocated with kzalloc so no data can leak apart
> > from previous readings.
> > 
> > Fixes: 3025c8688c1e ("iio: light: add support for UVIS25 sensor")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>  
> 
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

Unfortunately this is wrong (as Andy Schevchenko pointed out in one
of the other patches).  On x86_32 s64 is only aligned to 4 bytes
so the size of the structure will end up too small for the IIO alignment
rules.

There will be a v2 once we've figure out the best, consistent solution.

Sorry about that!

Jonathan

> 
> > ---
> >  drivers/iio/light/st_uvis25.h      | 5 +++++
> >  drivers/iio/light/st_uvis25_core.c | 6 +++---
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/st_uvis25.h b/drivers/iio/light/st_uvis25.h
> > index 78bc56aad129..f7027e4c4493 100644
> > --- a/drivers/iio/light/st_uvis25.h
> > +++ b/drivers/iio/light/st_uvis25.h
> > @@ -27,6 +27,11 @@ struct st_uvis25_hw {
> >  	struct iio_trigger *trig;
> >  	bool enabled;
> >  	int irq;
> > +	/* Ensure timestamp is naturally aligned */
> > +	struct {
> > +		u8 chan;
> > +		s64 ts;
> > +	} scan;
> >  };
> >  
> >  extern const struct dev_pm_ops st_uvis25_pm_ops;
> > diff --git a/drivers/iio/light/st_uvis25_core.c b/drivers/iio/light/st_uvis25_core.c
> > index d262c254b895..fe1f2dc970c7 100644
> > --- a/drivers/iio/light/st_uvis25_core.c
> > +++ b/drivers/iio/light/st_uvis25_core.c
> > @@ -234,17 +234,17 @@ static const struct iio_buffer_setup_ops st_uvis25_buffer_ops = {
> >  
> >  static irqreturn_t st_uvis25_buffer_handler_thread(int irq, void *p)
> >  {
> > -	u8 buffer[ALIGN(sizeof(u8), sizeof(s64)) + sizeof(s64)];
> >  	struct iio_poll_func *pf = p;
> >  	struct iio_dev *iio_dev = pf->indio_dev;
> >  	struct st_uvis25_hw *hw = iio_priv(iio_dev);
> >  	int err;
> >  
> > -	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR, (int *)buffer);
> > +	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR,
> > +			  (unsigned int *)&hw->scan.chan);
> >  	if (err < 0)
> >  		goto out;
> >  
> > -	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
> > +	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
> >  					   iio_get_time_ns(iio_dev));
> >  
> >  out:
> > -- 
> > 2.26.2
> >   
> 

Re: [PATCH 13/25] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.

[Jonathan Cameron]

On Mon, 25 May 2020 18:44:41 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:

> Hi Jonathan,
> 
> the driver is also doing other regmap_bulk_read and regmap_bulk_write for getting sensor data from registers and for setting hardware offsets.
> But there are numerous IIO drivers supporting SPI that are doing the same (IMU ST driver for example).
> 
> Are you sure this is really required? It seems to just call standard SPI transfers behind (I don't know if DMA can be implicitly call there)
> This is very inconvenient to deal with if there is no easy way to read/write multiple registers on the stack.

It's rare for this to be a problem on modern spi controllers, but there
are still some out there that will occasionally have trouble with
this. 

Personally I only ran into the problem in the wild once and it is really
really evil to track down.

We should clear the other cases out but it's not something I'd consider
urgent given no one is screaming about it.

> 
> Another thing I have in mind, the read for the FIFO should be replaced by a regmap_noinc_read, since this is a virtual register reading the FIFO, not the register map.

Good point.

Jonathan

> 
> Thanks,
> JB
> 
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, May 25, 2020 19:06
> To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> Subject: [PATCH 13/25] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues. 
>  
>  CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This case is a bit different to the rest of the series.  The driver
> was doing a regmap_bulk_read into a buffer that wasn't dma safe
> as it was on the stack with no guarantee of it being in a cacheline
> on it's own.   Fixing that also dealt with the data leak and
> alignment issues that Lars-Peter pointed out.
> 
> Also removed some unaligned handling as we are now aligned.
> 
> Fixes tag is for the dma safe buffer issue. Potentially we would
> need to backport timestamp alignment futher but that is a totally
> different patch.
> 
> Fixes: fd64df16f40e ("iio: imu: inv_mpu6050: Add SPI support for MPU6000")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 +++++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 12 ++++++------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index cd38b3fccc7b..e4df2d51b689 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -122,6 +122,9 @@ struct inv_mpu6050_chip_config {
>          u8 user_ctrl;
>  };
>  
> +/* 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8 */
> +#define INV_MPU6050_OUTPUT_DATA_SIZE         32
> +
>  /**
>   *  struct inv_mpu6050_hw - Other important hardware information.
>   *  @whoami:   Self identification byte from WHO_AM_I register
> @@ -165,6 +168,7 @@ struct inv_mpu6050_hw {
>   *  @magn_raw_to_gauss:        coefficient to convert mag raw value to Gauss.
>   *  @magn_orient:       magnetometer sensor chip orientation if available.
>   *  @suspended_sensors:        sensors mask of sensors turned off for suspend
> + *  @data:             dma safe buffer used for bulk reads.
>   */
>  struct inv_mpu6050_state {
>          struct mutex lock;
> @@ -190,6 +194,7 @@ struct inv_mpu6050_state {
>          s32 magn_raw_to_gauss[3];
>          struct iio_mount_matrix magn_orient;
>          unsigned int suspended_sensors;
> +       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE] ____cacheline_aligned;
>  };
>  
>  /*register and associated bit definition*/
> @@ -334,9 +339,6 @@ struct inv_mpu6050_state {
>  #define INV_ICM20608_TEMP_OFFSET             8170
>  #define INV_ICM20608_TEMP_SCALE              3059976
>  
> -/* 6 + 6 + 2 + 7 (for MPU9x50) = 21 round up to 24 and plus 8 */
> -#define INV_MPU6050_OUTPUT_DATA_SIZE         32
> -
>  #define INV_MPU6050_REG_INT_PIN_CFG     0x37
>  #define INV_MPU6050_ACTIVE_HIGH         0x00
>  #define INV_MPU6050_ACTIVE_LOW          0x80
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 9511e4715e2c..554c16592d47 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -13,7 +13,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/poll.h>
>  #include <linux/math64.h>
> -#include <asm/unaligned.h>
>  #include "inv_mpu_iio.h"
>  
>  /**
> @@ -121,7 +120,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>          struct inv_mpu6050_state *st = iio_priv(indio_dev);
>          size_t bytes_per_datum;
>          int result;
> -       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>          u16 fifo_count;
>          s64 timestamp;
>          int int_status;
> @@ -160,11 +158,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>           * read fifo_count register to know how many bytes are inside the FIFO
>           * right now
>           */
> -       result = regmap_bulk_read(st->map, st->reg->fifo_count_h, data,
> +       result = regmap_bulk_read(st->map, st->reg->fifo_count_h,
> +                                 st->data,
>                                    INV_MPU6050_FIFO_COUNT_BYTE);
>          if (result)
>                  goto end_session;
> -       fifo_count = get_unaligned_be16(&data[0]);
> +       fifo_count = be16_to_cpup((__be16 *)&st->data[0]);
>  
>          /*
>           * Handle fifo overflow by resetting fifo.
> @@ -182,7 +181,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>          inv_mpu6050_update_period(st, pf->timestamp, nb);
>          for (i = 0; i < nb; ++i) {
>                  result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> -                                         data, bytes_per_datum);
> +                                         st->data, bytes_per_datum);
>                  if (result)
>                          goto flush_fifo;
>                  /* skip first samples if needed */
> @@ -191,7 +190,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>                          continue;
>                  }
>                  timestamp = inv_mpu6050_get_timestamp(st);
> -               iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> +               iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> +                                                  timestamp);
>          }
>  
>  end_session:

Re: [PATCH 00/25] IIO: 2nd set of timestamp alignment fixes.

[Jonathan Cameron]

On off chance someone reads the cover letter. Note that there is a
big problem with both this and the previous series, so there will be a
v2.

Sorry about that an thank to Andy for a quick review.

Jonathan


On Mon, 25 May 2020 18:06:03 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Note there are still quite a few in the first set [2] that I have not
> picked up yet due to lack of review.   Any help with sanity checking
> those (and these) would be much appreciated.  These are not quite
> mechanical enough for me to be sure I haven't done anything silly
> so the more eyes the better.  Also see that discussion for extra
> details on why I went in various directions to fix this in the
> different drivers.
> 
> I've mostly avoided simply using __aligned(8) on the stack because
> it is only relatively recently that that has been guaranteed to work.
> Gcc 4.6 and kernel 4.19.  I'd like it to be sensible to backport
> most of these without significant rework. The mpl3115 is I think the
> only exception.
> 
> I also snuck in a warning fix for endian casting as it was in the code
> that I was touching to do the main fix.
> 
> So what's left from Lars list? 5 harder cases where I'm either fairly
> sure there are other bugs in the code, or I simply couldn't face
> figuring them out today.
> 
> Anyhow, to recap - round 1 cover letter.
> 
> Lars noted in a recent review [1] of the adis16475 that we had an issue around
> the alignment requirements of iio_push_to_buffers_with_timestamp.
> Whilst it's not documented, that function assumes that the overall buffer
> is 8 byte aligned, to ensure the timestamp is itself naturally aligned.
> We have drivers that use arrays (typically on the stack) that do
> not guarantee this alignment.
> 
> We could have fixed this by using a put_unaligned to write the timestamp
> but I think that just pushes the problem down the line.  If we were to
> have a consumer buffer wanting all the channels in the current
> active_scanmask then it will get the raw buffer from the driver passed
> straight through.  It seems odd to me if we allow passing a buffer
> that is not naturally aligned through to a consumer.
> Hence I'm proposing to fix up all existing drivers that might pass
> a buffer with insufficient alignment guarantees.
> Sometimes the timestamp is guaranteed to be in a particular location,
> in which case we can use C structure alignment guarantees to fix this
> in a nice readable fashion.  In other cases, the timestamp location
> depends on which channels are enabled, and in those case we can
> use explicit alignment __aligned(8) to ensure the whole array is
> appropriately aligned.
> 
> Lars-Peter also noted that, in many of these cases, there are holes
> in the stack array that we never write.  Those provide a potential
> leak of kernel data to userspace.  For drivers where this applies
> we either need to zero those holes each time, or allocate the buffer
> on the heap (only once), ensuring it is zeroed at that time.
> We may leak previous values from the sensor but currently that seems
> unlikely to present any form of security risk.
> 
> As such, this first set contains a mixture of fixes.  Where there
> are no possible holes, the buffer is kept on the stack but a
> c structure is used to guarantee appropriate alignment.  Where
> there are holes, the buffer is moved into the iio_priv() accessed
> data private structure. A c structure or __aligned(8) is used
> as appropriate.
> 
> I've stopped at this point rather than doing all the drivers Lars
> found in order to both throttle the review burden and also to
> see find any general problems with the fixes before doign futher
> similar series.  A few of the remaining ones will be rather more
> complex to deal with.
> 
> These have been there a long time, so whilst they are fixes we
> will want in stable I'm not that bothered if it takes us a little
> while to get them there!
> 
> [1] https://www.spinics.net/lists/devicetree/msg350590.html
> [2] https://patchwork.kernel.org/cover/11554215/
> 
> Jonathan Cameron (25):
>   iio:light:si1145: Fix timestamp alignment and prevent data leak.
>   iio:light:max44000 Fix timestamp alignment and prevent data leak.
>   iio:light:rpr0521 Fix timestamp alignment and prevent data leak.
>   iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.
>   iio:light:ltr501 Fix timestamp alignment issue.
>   iio:magnetometer:ak8974: Fix alignment and data leak issues
>   iio:magnetometer:ak8975 Fix alignment and data leak issues.
>   iio:magnetometer:mag3110 Fix alignment and data leak issues.
>   iio:humidity:hdc100x Fix alignment and data leak issues
>   iio:humidity:hts221 Fix alignment and data leak issues
>   iio:imu:bmi160 Fix alignment and data leak issues
>   iio:imu:st_lsm6dsx Fix alignment and data leak issues
>   iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.
>   iio:pressure:ms5611 Fix buffer element alignment
>   iio:pressure:mpl3115 Force alignment of buffer
>   iio:adc:ti-adc081c Fix alignment and data leak issues
>   iio:adc:ti-adc084s021 Fix alignment and data leak issues.
>   iio:adc:ti-adc084s021 Tidy up endian types
>   iio:adc:ti-ads1015 Fix buffer element alignment
>   iio:adc:ti-ads124s08 Fix alignment and data leak issues.
>   iio:adc:ti-ads8688 Fix alignment and potential data leak issue
>   iio:adc:ti-adc0832 Fix alignment issue with timestamp
>   iio:adc:ti-adc12138 Fix alignment issue with timestamp
>   iio:adc:ina2xx Fix timestamp alignment issue.
>   iio:adc:max1118 Fix alignment of timestamp and data leak issues
> 
>  drivers/iio/adc/ina2xx-adc.c                  |  8 +++---
>  drivers/iio/adc/max1118.c                     | 10 ++++---
>  drivers/iio/adc/ti-adc081c.c                  | 11 +++++---
>  drivers/iio/adc/ti-adc0832.c                  |  7 ++---
>  drivers/iio/adc/ti-adc084s021.c               | 20 ++++++++------
>  drivers/iio/adc/ti-adc12138.c                 |  9 ++++---
>  drivers/iio/adc/ti-ads1015.c                  | 12 ++++++---
>  drivers/iio/adc/ti-ads124s08.c                | 10 ++++---
>  drivers/iio/adc/ti-ads8688.c                  | 12 ++++++---
>  drivers/iio/humidity/hdc100x.c                | 10 ++++---
>  drivers/iio/humidity/hts221.h                 |  5 ++++
>  drivers/iio/humidity/hts221_buffer.c          |  9 ++++---
>  drivers/iio/imu/bmi160/bmi160.h               |  2 ++
>  drivers/iio/imu/bmi160/bmi160_core.c          |  5 ++--
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  8 +++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 12 ++++-----
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  5 ++++
>  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 27 ++++++++++---------
>  drivers/iio/light/ltr501.c                    | 15 ++++++-----
>  drivers/iio/light/max44000.c                  | 12 ++++++---
>  drivers/iio/light/rpr0521.c                   | 17 +++++++++---
>  drivers/iio/light/si1145.c                    |  7 ++---
>  drivers/iio/light/st_uvis25.h                 |  5 ++++
>  drivers/iio/light/st_uvis25_core.c            |  6 ++---
>  drivers/iio/magnetometer/ak8974.c             | 10 ++++---
>  drivers/iio/magnetometer/ak8975.c             | 20 +++++++++-----
>  drivers/iio/magnetometer/mag3110.c            | 13 ++++++---
>  drivers/iio/pressure/mpl3115.c                |  3 ++-
>  drivers/iio/pressure/ms5611_core.c            | 11 +++++---
>  29 files changed, 198 insertions(+), 103 deletions(-)
> 

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko]

On Tue, May 26, 2020 at 05:43:28PM +0100, Jonathan Cameron wrote:
> On Tue, 26 May 2020 12:15:56 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote:
> > > On 5/25/20 7:52 PM, Andy Shevchenko wrote:  
> > > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote:  
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>  

...

> > > > > +	struct {
> > > > > +		s16 channel;
> > > > > +		s64 ts;
> > > > > +	} scan;  
> > > > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from
> > > > iio_push_to_buffers_with_timestamp() point of view?  
> > > 
> > > No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks
> > > like we can't rely on implicit padding, but need to always explicitly
> > > specify it.
> > > 
> > > Or maybe we can typedef and IIO timestamp type with an explicit __aligned
> > > attribute. I wonder if that works... After having a quick look, the kernel
> > > already defines aligned_u64, so maybe using that is an option.  
> > 
> > Another way is simple to provide offset of timestamp member as a parameter.
> > Though, if it's an ABI, then alas, we need to align it properly.
> > 
> > Also, wouldn't be better to explicitly show the padding?
> > 
> > 	struct {
> > 		s16 channel;
> > 		s16 padding[3];
> > 		s64 ts;
> > 	} scan;
> > 
> > (matter of style though, just saying).
> > 
> 
> gah.  Thanks for pointing this out Andy.  I wanted to avoid explicitly
> calling out empty padding because it seemed to me to be more likely to
> be error prone than filling it in.
> 
> I was trying to avoid using __aligned on the stack as it only works for
> more recent kernels (due to gcc version changes) and some of these predate
> that point.
> 
> I guess we just do it explicitly in all these cases.
> 
> The two patches that have already gone to Greg both have sufficient
> data to ensure the structure is big enough (only 16 bytes padding in one and
> none in the other).
> 
> I think we are also fine for the original question as well as it won't
> matter if the whole structure is aligned to 4 bytes on x86_32 and
> similar as an 8 byte write will be fine.
> 
> So fun question - do we want to enforce 8 byte alignment of the whole
> structure, or simply the padding?
> 
> Maybe better to just do the padding explicitly as Andy suggested.

I have talked to colleague of mine, and we concluded (but without any
documentation proved evidence, one needs basically to read C standard followed
by ABI of all architectures supported by Linux) that the following will work.

Consider your patch, which introduces natural alignment via struct:

	struct scan {
		s16 ...;
		...
		s64 ts;
	};

When we access ts as struct member like scan->ts, compiler makes sure that
there will be no hardware exception (due to unaligned access). Now, we _assume_
that dereferencing like

	void *buf = &scan;
	(int64_t *)buf[ts_offset] = value;

will work flawlessly because above.

If it's indeed a case, what we simple need is to pass ts offset into
iio_push_to_buffers_with_timestamp().

If it's not the case, we _additionally_ will need to replace
	(int64_t *)buf[ts_offset] = value;
by
	put_unaligned(value, (int64_t *)...);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Jonathan Cameron]

On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, May 26, 2020 at 05:43:28PM +0100, Jonathan Cameron wrote:
>> On Tue, 26 May 2020 12:15:56 +0300
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote:
>> > > On 5/25/20 7:52 PM, Andy Shevchenko wrote:  
>> > > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron
>wrote:  
>> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
>
>...
>
>> > > > > +	struct {
>> > > > > +		s16 channel;
>> > > > > +		s64 ts;
>> > > > > +	} scan;  
>> > > > Hmm... On x86_32 and x86_64 this will give different padding.
>Is it okay from
>> > > > iio_push_to_buffers_with_timestamp() point of view?  
>> > > 
>> > > No, this is terrible. IIO expects 64 bit words to be 64 bit
>aligned. Looks
>> > > like we can't rely on implicit padding, but need to always
>explicitly
>> > > specify it.
>> > > 
>> > > Or maybe we can typedef and IIO timestamp type with an explicit
>__aligned
>> > > attribute. I wonder if that works... After having a quick look,
>the kernel
>> > > already defines aligned_u64, so maybe using that is an option.  
>> > 
>> > Another way is simple to provide offset of timestamp member as a
>parameter.
>> > Though, if it's an ABI, then alas, we need to align it properly.
>> > 
>> > Also, wouldn't be better to explicitly show the padding?
>> > 
>> > 	struct {
>> > 		s16 channel;
>> > 		s16 padding[3];
>> > 		s64 ts;
>> > 	} scan;
>> > 
>> > (matter of style though, just saying).
>> > 
>> 
>> gah.  Thanks for pointing this out Andy.  I wanted to avoid
>explicitly
>> calling out empty padding because it seemed to me to be more likely
>to
>> be error prone than filling it in.
>> 
>> I was trying to avoid using __aligned on the stack as it only works
>for
>> more recent kernels (due to gcc version changes) and some of these
>predate
>> that point.
>> 
>> I guess we just do it explicitly in all these cases.
>> 
>> The two patches that have already gone to Greg both have sufficient
>> data to ensure the structure is big enough (only 16 bytes padding in
>one and
>> none in the other).
>> 
>> I think we are also fine for the original question as well as it
>won't
>> matter if the whole structure is aligned to 4 bytes on x86_32 and
>> similar as an 8 byte write will be fine.
>> 
>> So fun question - do we want to enforce 8 byte alignment of the whole
>> structure, or simply the padding?
>> 
>> Maybe better to just do the padding explicitly as Andy suggested.
>
>I have talked to colleague of mine, and we concluded (but without any
>documentation proved evidence, one needs basically to read C standard
>followed
>by ABI of all architectures supported by Linux) that the following will
>work.
>
>Consider your patch, which introduces natural alignment via struct:
>
>	struct scan {
>		s16 ...;
>		...
>		s64 ts;
>	};
>
>When we access ts as struct member like scan->ts, compiler makes sure
>that
>there will be no hardware exception (due to unaligned access). Now, we
>_assume_
>that dereferencing like
>
>	void *buf = &scan;
>	(int64_t *)buf[ts_offset] = value;
>
>will work flawlessly because above.
>
>If it's indeed a case, what we simple need is to pass ts offset into
>iio_push_to_buffers_with_timestamp().

Realistically it might work but doesn't help us. We need all receivers of that buffer to
 know the location of ts.  It has always been implicit like any other channel. 
 Rules are natural alignment so alignment is same as size of element. 

Hence we need to ensure padding, but I'm not sure about alignment.  If padding was
right for 8 byte alignment but whole structure had 4 byte alignment then I think we
 should be fine. So slightly more relaxed the ensuring ts is 8 byte aligned. 

Might be easier to just align it though than explain this subtlety. 

J


>
>If it's not the case, we _additionally_ will need to replace
>	(int64_t *)buf[ts_offset] = value;
>by
>	put_unaligned(value, (int64_t *)...);

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 09/25] iio:humidity:hdc100x Fix alignment and data leak issues

[Matt Ranostay]

On Mon, May 25, 2020 at 10:09 AM 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.
>

Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

> Fixes: 16bf793f86b2 ("iio: humidity: hdc100x: add triggered buffer support for HDC100X")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Alison Schofield <amsfield22@gmail.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/humidity/hdc100x.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> index 7ecd2ffa3132..fd825e281d4f 100644
> --- a/drivers/iio/humidity/hdc100x.c
> +++ b/drivers/iio/humidity/hdc100x.c
> @@ -38,6 +38,11 @@ struct hdc100x_data {
>
>         /* integration time of the sensor */
>         int adc_int_us[2];
> +       /* Ensure natural alignment of timestamp */
> +       struct {
> +               __be16 channels[2];
> +               s64 ts;
> +       } scan;
>  };
>
>  /* integration time in us */
> @@ -322,7 +327,6 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
>         struct i2c_client *client = data->client;
>         int delay = data->adc_int_us[0] + data->adc_int_us[1];
>         int ret;
> -       s16 buf[8];  /* 2x s16 + padding + 8 byte timestamp */
>
>         /* dual read starts at temp register */
>         mutex_lock(&data->lock);
> @@ -333,13 +337,13 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
>         }
>         usleep_range(delay, delay + 1000);
>
> -       ret = i2c_master_recv(client, (u8 *)buf, 4);
> +       ret = i2c_master_recv(client, (u8 *)data->scan.channels, 4);
>         if (ret < 0) {
>                 dev_err(&client->dev, "cannot read sensor data\n");
>                 goto err;
>         }
>
> -       iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +       iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
>                                            iio_get_time_ns(indio_dev));
>  err:
>         mutex_unlock(&data->lock);
> --
> 2.26.2
>

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko]

On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote:
> On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Might be easier to just align it though than explain this subtlety. 

The easiest one seems to move ts to be first member of the struct / buffer. Problem solved.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Jonathan Cameron]

On Wed, 27 May 2020 00:03:13 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote:
> > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> 
> > Might be easier to just align it though than explain this subtlety.   
> 
> The easiest one seems to move ts to be first member of the struct / buffer. Problem solved.

Sort of, but it brings other problems.

1) Breaks userspace ABI.  Yes, no one should be relying on the ordering of
   elements from a given sensor, but we all know someone will be.   As
   things currently stand note we only have one actual report of there
   being a case where the alignment stuff breaks down (before I broke it
   much more significantly with this patch set!)

2) We have to rework all the drivers, including the majority that use a suitably
   aligned buffer anyway (typically ____cacheline_aligned for DMA safety).
   The structure thing doesn't work any more because the timestamp is optional,
   so we have to have the drivers do their alignment differently depending on whether
   it is there or not, so we are back to use using __aligned(8) for all the buffers. 
   At the end of the day, the iio_push_to_buffers_with_timestamp is only workable
   because the timestamp is at the end of the buffer.

At this point I'm thinking we just stick to u8, u16 etc arrays.  At that point
we either decide that we are happy to not backport past (IIRC) 4.19 where the minimum gcc
version was increased such that __aligned(8) works on the stack, or we move them into
the iio_priv() structure where __aligned(8) always worked and hence squash the issue
of kernel data leaking without a memset on each scan. The only remaining question is
whether we get extra clarity by using

struct {
	s16 channels[2];
	// I think we can actually drop the padding if marking the timestamp as
	// __aligned(8)
	u8 padding[4];
	s64 timestamp __aligned(8);
} scan;

or 

s16 scan[8] __aligned(8);


For the __aligned part this from the gcc docs looks helpful:

https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Type-Attributes.html

"Note that the alignment of any given struct or union type is
required by the ISO C standard to be at least a perfect multiple
of the lowest common multiple of the alignments of all of the
members of the struct or union in question. This means that you
can effectively adjust the alignment of a struct or union type by
attaching an aligned attribute to any one of the members of such a
type, but the notation illustrated in the example above is a more
obvious, intuitive, and readable way to request the compiler to
adjust the alignment of an entire struct or union type. "

So I think that means we can safely do

struct {
	u8 channel;
	s64 ts __aligned(8);
};

and be guaranteed correct padding and alignment in what I think is
a fairly readable form.  It's also self documenting so I can
probably drop some of the explanatory comments.

Jonathan

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko]

On Wed, May 27, 2020 at 12:41:07PM +0100, Jonathan Cameron wrote:
> On Wed, 27 May 2020 00:03:13 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote:
> > > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  

> > > Might be easier to just align it though than explain this subtlety.   
> > 
> > The easiest one seems to move ts to be first member of the struct / buffer. Problem solved.
> 
> Sort of, but it brings other problems.
> 
> 1) Breaks userspace ABI.  Yes, no one should be relying on the ordering of
>    elements from a given sensor, but we all know someone will be.   As
>    things currently stand note we only have one actual report of there
>    being a case where the alignment stuff breaks down (before I broke it
>    much more significantly with this patch set!)
> 
> 2) We have to rework all the drivers, including the majority that use a suitably
>    aligned buffer anyway (typically ____cacheline_aligned for DMA safety).
>    The structure thing doesn't work any more because the timestamp is optional,
>    so we have to have the drivers do their alignment differently depending on whether
>    it is there or not, so we are back to use using __aligned(8) for all the buffers. 
>    At the end of the day, the iio_push_to_buffers_with_timestamp is only workable
>    because the timestamp is at the end of the buffer.

I see.

> At this point I'm thinking we just stick to u8, u16 etc arrays.  At that point
> we either decide that we are happy to not backport past (IIRC) 4.19 where the minimum gcc
> version was increased such that __aligned(8) works on the stack, or we move them into
> the iio_priv() structure where __aligned(8) always worked and hence squash the issue
> of kernel data leaking without a memset on each scan. The only remaining question is
> whether we get extra clarity by using
> 
> struct {
> 	s16 channels[2];
> 	// I think we can actually drop the padding if marking the timestamp as
> 	// __aligned(8)
> 	u8 padding[4];
> 	s64 timestamp __aligned(8);
> } scan;
> 
> or 
> 
> s16 scan[8] __aligned(8);
> 
> 
> For the __aligned part this from the gcc docs looks helpful:
> 
> https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Type-Attributes.html
> 
> "Note that the alignment of any given struct or union type is
> required by the ISO C standard to be at least a perfect multiple
> of the lowest common multiple of the alignments of all of the
> members of the struct or union in question. This means that you
> can effectively adjust the alignment of a struct or union type by
> attaching an aligned attribute to any one of the members of such a
> type, but the notation illustrated in the example above is a more
> obvious, intuitive, and readable way to request the compiler to
> adjust the alignment of an entire struct or union type. "

This means that

struct sx {
	u8 channel;
	s64 ts;
};

struct sx y, *py = &y;

py will be always aligned to at least 8 bytes (per s64 type).
It has no effect on the members themselves.

> So I think that means we can safely do
> 
> struct {
> 	u8 channel;
> 	s64 ts __aligned(8);
> };

I don't know how this attribute will affect *a member* of the struct. Perhaps
it's straightforward and GCC simple apply it to POD.

> and be guaranteed correct padding and alignment in what I think is
> a fairly readable form.  It's also self documenting so I can
> probably drop some of the explanatory comments.

If it is documented, then I fully agree, otherwise will look
like a (fragile) hack.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Andy Shevchenko]

On Wed, May 27, 2020 at 03:37:13PM +0300, Andy Shevchenko wrote:
> On Wed, May 27, 2020 at 12:41:07PM +0100, Jonathan Cameron wrote:
> > On Wed, 27 May 2020 00:03:13 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > So I think that means we can safely do
> > 
> > struct {
> > 	u8 channel;
> > 	s64 ts __aligned(8);
> > };
> 
> I don't know how this attribute will affect *a member* of the struct. Perhaps
> it's straightforward and GCC simple apply it to POD.
> 
> > and be guaranteed correct padding and alignment in what I think is
> > a fairly readable form.  It's also self documenting so I can
> > probably drop some of the explanatory comments.
> 
> If it is documented, then I fully agree, otherwise will look
> like a (fragile) hack.

I meant here if __aligned() behaviour against POD inside the structure
is documented in GCC manuals / etc.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 19/25] iio:adc:ti-ads1015 Fix buffer element alignment

[Jonathan Cameron]

On Wed, 27 May 2020 15:37:13 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, May 27, 2020 at 12:41:07PM +0100, Jonathan Cameron wrote:
> > On Wed, 27 May 2020 00:03:13 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote:  
> > > > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:    
> 
> > > > Might be easier to just align it though than explain this subtlety.     
> > > 
> > > The easiest one seems to move ts to be first member of the struct / buffer. Problem solved.  
> > 
> > Sort of, but it brings other problems.
> > 
> > 1) Breaks userspace ABI.  Yes, no one should be relying on the ordering of
> >    elements from a given sensor, but we all know someone will be.   As
> >    things currently stand note we only have one actual report of there
> >    being a case where the alignment stuff breaks down (before I broke it
> >    much more significantly with this patch set!)
> > 
> > 2) We have to rework all the drivers, including the majority that use a suitably
> >    aligned buffer anyway (typically ____cacheline_aligned for DMA safety).
> >    The structure thing doesn't work any more because the timestamp is optional,
> >    so we have to have the drivers do their alignment differently depending on whether
> >    it is there or not, so we are back to use using __aligned(8) for all the buffers. 
> >    At the end of the day, the iio_push_to_buffers_with_timestamp is only workable
> >    because the timestamp is at the end of the buffer.  
> 
> I see.
> 
> > At this point I'm thinking we just stick to u8, u16 etc arrays.  At that point
> > we either decide that we are happy to not backport past (IIRC) 4.19 where the minimum gcc
> > version was increased such that __aligned(8) works on the stack, or we move them into
> > the iio_priv() structure where __aligned(8) always worked and hence squash the issue
> > of kernel data leaking without a memset on each scan. The only remaining question is
> > whether we get extra clarity by using
> > 
> > struct {
> > 	s16 channels[2];
> > 	// I think we can actually drop the padding if marking the timestamp as
> > 	// __aligned(8)
> > 	u8 padding[4];
> > 	s64 timestamp __aligned(8);
> > } scan;
> > 
> > or 
> > 
> > s16 scan[8] __aligned(8);
> > 
> > 
> > For the __aligned part this from the gcc docs looks helpful:
> > 
> > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Type-Attributes.html
> > 
> > "Note that the alignment of any given struct or union type is
> > required by the ISO C standard to be at least a perfect multiple
> > of the lowest common multiple of the alignments of all of the
> > members of the struct or union in question. This means that you
> > can effectively adjust the alignment of a struct or union type by
> > attaching an aligned attribute to any one of the members of such a
> > type, but the notation illustrated in the example above is a more
> > obvious, intuitive, and readable way to request the compiler to
> > adjust the alignment of an entire struct or union type. "  
> 
> This means that
> 
> struct sx {
> 	u8 channel;
> 	s64 ts;
> };
> 
> struct sx y, *py = &y;
> 
> py will be always aligned to at least 8 bytes (per s64 type).
> It has no effect on the members themselves.

Ah. I cut too much out. 

"This attribute specifies a minimum alignment (in bytes) for variables of the specified type."

So taking...

struct sx {
	u8 channel;
	s64 ts __aligned(8);
};

So if ts is __aligned(8) it will be aligned to 8 bytes, but so will the structure containing it.
hence will will have 7 bytes of padding.

There are a few similar users of this in kernel.  For example

https://elixir.bootlin.com/linux/latest/source/include/linux/efi.h#L809

Which after digging in the UEFI spec is because all variables in UEFI are
defined to have natural alignment and the structures are aligned to the
largest size.  So effectively same case we have.

Lots of other cases, that one just had a convenient spec to point at.
They mostly exist in uapi headers presumably to allow stuff to consistent
across from x86_64 to x86_32 userspace.


> 
> > So I think that means we can safely do
> > 
> > struct {
> > 	u8 channel;
> > 	s64 ts __aligned(8);
> > };  
> 
> I don't know how this attribute will affect *a member* of the struct. Perhaps
> it's straightforward and GCC simple apply it to POD.
> 
> > and be guaranteed correct padding and alignment in what I think is
> > a fairly readable form.  It's also self documenting so I can
> > probably drop some of the explanatory comments.  
> 
> If it is documented, then I fully agree, otherwise will look
> like a (fragile) hack.
> 

Agreed.  It seems to be documented and I'm reassured by the bunch
of existing places this seems to be being used.
Also helps that they are mostly for uapi consistency which is
really want we are talking about here as well.

Will let this sit for a few days before I do anything about a v2
in the hope that someone will point out any more corner cases
I should have known about!

Jonathan

Re: [PATCH 04/25] iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Tue, 26 May 2020 17:53:26 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 26 May 2020 09:55:05 +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 suitable structure in the iio_priv()
> > > 
> > > This data is allocated with kzalloc so no data can leak apart
> > > from previous readings.
> > > 
> > > Fixes: 3025c8688c1e ("iio: light: add support for UVIS25 sensor")
> > > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>    
> > 
> > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>  
> 
> Unfortunately this is wrong (as Andy Schevchenko pointed out in one
> of the other patches).  On x86_32 s64 is only aligned to 4 bytes
> so the size of the structure will end up too small for the IIO alignment
> rules.
> 
> There will be a v2 once we've figure out the best, consistent solution.

Given the change is fairly small I've kept acks etc from V1. Hopefully
no one minds that.

Jonathan

> 
> Sorry about that!
> 
> Jonathan
> 
> >   
> > > ---
> > >  drivers/iio/light/st_uvis25.h      | 5 +++++
> > >  drivers/iio/light/st_uvis25_core.c | 6 +++---
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/light/st_uvis25.h b/drivers/iio/light/st_uvis25.h
> > > index 78bc56aad129..f7027e4c4493 100644
> > > --- a/drivers/iio/light/st_uvis25.h
> > > +++ b/drivers/iio/light/st_uvis25.h
> > > @@ -27,6 +27,11 @@ struct st_uvis25_hw {
> > >  	struct iio_trigger *trig;
> > >  	bool enabled;
> > >  	int irq;
> > > +	/* Ensure timestamp is naturally aligned */
> > > +	struct {
> > > +		u8 chan;
> > > +		s64 ts;
> > > +	} scan;
> > >  };
> > >  
> > >  extern const struct dev_pm_ops st_uvis25_pm_ops;
> > > diff --git a/drivers/iio/light/st_uvis25_core.c b/drivers/iio/light/st_uvis25_core.c
> > > index d262c254b895..fe1f2dc970c7 100644
> > > --- a/drivers/iio/light/st_uvis25_core.c
> > > +++ b/drivers/iio/light/st_uvis25_core.c
> > > @@ -234,17 +234,17 @@ static const struct iio_buffer_setup_ops st_uvis25_buffer_ops = {
> > >  
> > >  static irqreturn_t st_uvis25_buffer_handler_thread(int irq, void *p)
> > >  {
> > > -	u8 buffer[ALIGN(sizeof(u8), sizeof(s64)) + sizeof(s64)];
> > >  	struct iio_poll_func *pf = p;
> > >  	struct iio_dev *iio_dev = pf->indio_dev;
> > >  	struct st_uvis25_hw *hw = iio_priv(iio_dev);
> > >  	int err;
> > >  
> > > -	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR, (int *)buffer);
> > > +	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR,
> > > +			  (unsigned int *)&hw->scan.chan);
> > >  	if (err < 0)
> > >  		goto out;
> > >  
> > > -	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
> > > +	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
> > >  					   iio_get_time_ns(iio_dev));
> > >  
> > >  out:
> > > -- 
> > > 2.26.2
> > >     
> >   
> 
> 

Re: [PATCH 06/25] iio:magnetometer:ak8974: Fix alignment and data leak issues

[Jonathan Cameron]

On Tue, 26 May 2020 11:24:40 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, May 25, 2020 at 7:09 PM 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 appart from
> > previous readings.
> >
> > Fixes: 7c94a8b2ee8cf ("iio: magn: add a driver for AK8974")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>  
> 
> Whoa, good catch! Definitely my mindless coding.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
I've tweaked this slightly fro v2 to add an __aligned(8) to the ts.
This is driven by the need for some cases to be careful on x86_32
where the s64 might be 4 byte aligned and the padding come out wrong.
It doesn't actually matter in this case but I'd rather be explicit.

Have kept the reviewed-by as not a material change on this one.

Thanks,

Jonathan

> 
> Yours,
> Linus Walleij

Re: [PATCH 09/25] iio:humidity:hdc100x Fix alignment and data leak issues

[Jonathan Cameron]

On Tue, 26 May 2020 12:31:11 -0700
Matt Ranostay <matt.ranostay@konsulko.com> wrote:

> On Mon, May 25, 2020 at 10:09 AM 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.
> >  
> 
> Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

This is one of the cases that Andy pointed out will be inconsistent
on x86_32.  That only requires 4 byte alignment for the s64 so here
we end up with the wrong amount of padding.  I've fixed that up
in v2 with s64 ts __aligned(8);

I've assumed the ack still stands given this is a fairly obscure corner.

Thanks,

Jonathan

> 
> > Fixes: 16bf793f86b2 ("iio: humidity: hdc100x: add triggered buffer support for HDC100X")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Alison Schofield <amsfield22@gmail.com>
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/iio/humidity/hdc100x.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> > index 7ecd2ffa3132..fd825e281d4f 100644
> > --- a/drivers/iio/humidity/hdc100x.c
> > +++ b/drivers/iio/humidity/hdc100x.c
> > @@ -38,6 +38,11 @@ struct hdc100x_data {
> >
> >         /* integration time of the sensor */
> >         int adc_int_us[2];
> > +       /* Ensure natural alignment of timestamp */
> > +       struct {
> > +               __be16 channels[2];
> > +               s64 ts;
> > +       } scan;
> >  };
> >
> >  /* integration time in us */
> > @@ -322,7 +327,6 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
> >         struct i2c_client *client = data->client;
> >         int delay = data->adc_int_us[0] + data->adc_int_us[1];
> >         int ret;
> > -       s16 buf[8];  /* 2x s16 + padding + 8 byte timestamp */
> >
> >         /* dual read starts at temp register */
> >         mutex_lock(&data->lock);
> > @@ -333,13 +337,13 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
> >         }
> >         usleep_range(delay, delay + 1000);
> >
> > -       ret = i2c_master_recv(client, (u8 *)buf, 4);
> > +       ret = i2c_master_recv(client, (u8 *)data->scan.channels, 4);
> >         if (ret < 0) {
> >                 dev_err(&client->dev, "cannot read sensor data\n");
> >                 goto err;
> >         }
> >
> > -       iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > +       iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> >                                            iio_get_time_ns(indio_dev));
> >  err:
> >         mutex_unlock(&data->lock);
> > --
> > 2.26.2
> >  

Re: [PATCH 10/25] iio:humidity:hts221 Fix alignment and data leak issues

[Jonathan Cameron]

On Tue, 26 May 2020 09:52:30 +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 suitable structure in the iio_priv() data.
> > This data is allocated with kzalloc so no data can leak
> > apart from previous readings.
> > 
> > Fixes: e4a70e3e7d84 ("iio: humidity: add support to hts221 rh/temp combo device")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>  
> 
> Hi Jonathan,
> 
> I guess you can drop HTS221_DATA_SIZE now since it seems no longer used.
> 
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

This patch suffers from the issue with padding on x86_32 that
Andy pointed out.  For v2 I've added explicit __aligned(8)

Also dropped HTS221_DATA_SIZE as you suggested.

I've kept the Ack. Shout if you'd rather I didn't!

Jonathan

> 
> > ---
> >  drivers/iio/humidity/hts221.h        | 5 +++++
> >  drivers/iio/humidity/hts221_buffer.c | 9 +++++----
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
> > index 7c650df77556..6ad579ca9cef 100644
> > --- a/drivers/iio/humidity/hts221.h
> > +++ b/drivers/iio/humidity/hts221.h
> > @@ -39,6 +39,11 @@ struct hts221_hw {
> >  
> >  	bool enabled;
> >  	u8 odr;
> > +	/* Ensure natural alignment of timestamp */
> > +	struct {
> > +		__le16 channels[2];
> > +		s64 ts;
> > +	} scan;
> >  };
> >  
> >  extern const struct dev_pm_ops hts221_pm_ops;
> > diff --git a/drivers/iio/humidity/hts221_buffer.c b/drivers/iio/humidity/hts221_buffer.c
> > index 21c6c160462d..59ede9860185 100644
> > --- a/drivers/iio/humidity/hts221_buffer.c
> > +++ b/drivers/iio/humidity/hts221_buffer.c
> > @@ -160,7 +160,6 @@ static const struct iio_buffer_setup_ops hts221_buffer_ops = {
> >  
> >  static irqreturn_t hts221_buffer_handler_thread(int irq, void *p)
> >  {
> > -	u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)];
> >  	struct iio_poll_func *pf = p;
> >  	struct iio_dev *iio_dev = pf->indio_dev;
> >  	struct hts221_hw *hw = iio_priv(iio_dev);
> > @@ -170,18 +169,20 @@ static irqreturn_t hts221_buffer_handler_thread(int irq, void *p)
> >  	/* humidity data */
> >  	ch = &iio_dev->channels[HTS221_SENSOR_H];
> >  	err = regmap_bulk_read(hw->regmap, ch->address,
> > -			       buffer, HTS221_DATA_SIZE);
> > +			       &hw->scan.channels[0],
> > +			       sizeof(hw->scan.channels[0]));
> >  	if (err < 0)
> >  		goto out;
> >  
> >  	/* temperature data */
> >  	ch = &iio_dev->channels[HTS221_SENSOR_T];
> >  	err = regmap_bulk_read(hw->regmap, ch->address,
> > -			       buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE);
> > +			       &hw->scan.channels[1],
> > +			       sizeof(hw->scan.channels[1]));
> >  	if (err < 0)
> >  		goto out;
> >  
> > -	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
> > +	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
> >  					   iio_get_time_ns(iio_dev));
> >  
> >  out:
> > -- 
> > 2.26.2
> >   

Re: [PATCH 12/25] iio:imu:st_lsm6dsx Fix alignment and data leak issues

[Jonathan Cameron]

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
> >