[PATCH v3 00/27] IIO: Fused set 1 and 2 of timestamp alignment fixes

[PATCH v3 00/27] IIO: Fused set 1 and 2 of timestamp alignment fixes

[Jonathan Cameron]

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

These are back again. Please take a look at them.  I'm fairly sure these
are all 'safe' but really don't like taking patches without the surety of
a second pair of eyes having taken a look at them. That is particularly
true of fixes for long term issues like these ones.

Changes since v2:
* bmc150-accel: Use sizeof() for channel size (Andy Shevchenko)
* st_uvis25: Use local variable for regmap call (Andy Shevchenko)
* st_lsm6dsx: Use array of scan[] rather than 3 structures (Lorenzo Bianconi)
* inv_mpu6050: Add patch switching to a regmap_noinc_read (Jean-Baptiste Maneyrol)
* ina2xx: Use a structure (previously failed to notice that works here)
* I've added clarifying notes to patch descriptions based on questions asked.
  These were mainly about why we didn't use the structure approach everywhere
  and why I've forced alignment in places it wasn't strictly needed.

Previous cover letter:
A few notes based on questions on v1.

1. Why not use put_unaligned to avoid the whole thing?
   This interface can pass directly to a variety of in kernel users
   who would reasonably assume that we have true natural alignment.
   When it gets passed directly is subtle as it depends on whether
   the demux has to realign the data or not.  So enabling an extra
   channel could result in a previously working alignment no longer
   being true.
   
   Even if this is fine for existing usecases we are likely to
   store up subtle long term issues if we don't fix it explicitly.
   It's also worth noting that the data channel sometimes suffered
   the same problem as the timestamp.

2. Why not specify explicit padding?
   In my view this is error prone in comparisom with relying on
   c to do the hard work for us.

3. Why not move the timestamp to the start?
   ABI breakage and as timestamp is optional (no obvious from the
   iio_push_to_buffers_with_timestamp call) we can end up having
   to shift the rest of the data within that call.
   
Changes since v1.

Andy Schevchenko pointed out that on x86_32 s64 elements are only
aligned to 4 bytes.  Where I had tried to use a structure to avoid
explicit need to list the padding, there were some cases where
this results in insufficient padding being inserted.

This doesn't affect the few patches that had already been applied and
sent upstream. (which was lucky ;)

The fix was to take advantage of __aligned(8) which (according to
my reading of the c spec and the gcc docs) enforces the alignment of
both the element within a structure and the structure itself.
The kernel now requires a recent enough version of GCC to ensure this
works both on the stack and heap.  This is done in lots of other
userspace interfaces. In some cases iio_push_to_buffers_with_ts
is aligning data for passing to userspace, be it via a kfifo
so it is sensible we should use the same solution.

Note that we could have used u64_aligned but there is no equivalent
for s64 and explicit use of __aligned(8) is common in
the kernel so we adopt this here.

Note that there were about 8 drivers that would have been broken with
v1 of the patch.  I have also forced alignment of timestamps in cases
where (mostly by coincidence) we would have been fine (padding was
less than 4 bytes anyway.  I did this partly to reduce fragility if
other elements are added in future and also to avoid cut and paste
errors in new drivers.

There were a few other minor tidying up changes inline with reviews
of v1.

I've kept tags given for v1 on basis the changes are minor. Shout if
you disagree.

Version 1 part 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 (27):
  iio: accel: kxsd9: Fix alignment of local buffer.
  iio:accel:mma8452: Fix timestamp alignment and prevent data leak.
  iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak.
  iio:accel:mma7455: Fix timestamp alignment and prevent data leak.
  iio:gyro:itg3200: Fix timestamp alignment and prevent data leak.
  iio:proximity:mb1232: Fix timestamp alignment and prevent data leak.
  iio:chemical:ccs811: Fix timestamp alignment and prevent data leak.
  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:ak8975 Fix alignment and data leak issues.
  iio:magnetometer:mag3110 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:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.
  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-ads124s08 Fix alignment and data leak issues.
  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/accel/bmc150-accel-core.c         | 15 ++++++--
 drivers/iio/accel/kxsd9.c                     | 16 ++++++---
 drivers/iio/accel/mma7455_core.c              | 16 ++++++---
 drivers/iio/accel/mma8452.c                   | 11 ++++--
 drivers/iio/adc/ina2xx-adc.c                  | 11 +++---
 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-ads124s08.c                | 10 ++++--
 drivers/iio/chemical/ccs811.c                 | 13 ++++---
 drivers/iio/gyro/itg3200_buffer.c             | 15 +++++---
 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    | 14 ++++----
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  6 ++++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 36 ++++++++++---------
 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            |  8 +++--
 drivers/iio/magnetometer/ak8975.c             | 16 ++++++---
 drivers/iio/magnetometer/mag3110.c            | 13 ++++---
 drivers/iio/pressure/mpl3115.c                |  3 +-
 drivers/iio/proximity/mb1232.c                | 17 ++++-----
 29 files changed, 229 insertions(+), 119 deletions(-)

-- 
2.27.0

[PATCH v3 01/27] iio: accel: kxsd9: Fix alignment of local buffer.

[Jonathan Cameron]

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

iio_push_to_buffers_with_timestamp assumes 8 byte alignment which
is not guaranteed by an array of smaller elements.

Note that whilst in this particular case the alignment forcing
of the ts element is not strictly necessary it acts as good
documentation.  Doing this where not necessary should cut
down on the number of cut and paste introduced errors elsewhere.

Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/kxsd9.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 66b2e4cf24cf..0e18b92e2099 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -209,14 +209,20 @@ static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
 	const struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct kxsd9_state *st = iio_priv(indio_dev);
+	/*
+	 * Ensure correct positioning and alignment of timestamp.
+	 * No need to zero initialize as all elements written.
+	 */
+	struct {
+		__be16 chan[4];
+		s64 ts __aligned(8);
+	} hw_values;
 	int ret;
-	/* 4 * 16bit values AND timestamp */
-	__be16 hw_values[8];
 
 	ret = regmap_bulk_read(st->map,
 			       KXSD9_REG_X,
-			       &hw_values,
-			       8);
+			       hw_values.chan,
+			       sizeof(hw_values.chan));
 	if (ret) {
 		dev_err(st->dev,
 			"error reading data\n");
@@ -224,7 +230,7 @@ static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
 	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev,
-					   hw_values,
+					   &hw_values,
 					   iio_get_time_ns(indio_dev));
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.27.0

[PATCH v3 02/27] iio:accel:mma8452: 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 u8 array on the stack.  As Lars also noted
this anti pattern can involve a leak of data to userspace and that
indeed can happen here.  We close both issues by moving to
a suitable structure in the iio_priv() data with alignment
ensured by use of an explicit c structure.  This data is allocated
with kzalloc so no data can leak appart from previous readings.

The additional forcing of the 8 byte alignment of the timestamp
is not strictly necessary but makes the code less fragile by
making this explicit.

Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma8452.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index ba27f8673131..1cf2b5db26ca 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -110,6 +110,12 @@ struct mma8452_data {
 	int sleep_val;
 	struct regulator *vdd_reg;
 	struct regulator *vddio_reg;
+
+	/* Ensure correct alignment of time stamp when present */
+	struct {
+		__be16 channels[3];
+		s64 ts __aligned(8);
+	} buffer;
 };
 
  /**
@@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mma8452_data *data = iio_priv(indio_dev);
-	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
 	int ret;
 
-	ret = mma8452_read(data, (__be16 *)buffer);
+	ret = mma8452_read(data, data->buffer.channels);
 	if (ret < 0)
 		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.27.0

[PATCH v3 03/27] iio:accel:bmc150-accel: 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() data with alignment
ensured by use of an explicit c structure.  This data is allocated
with kzalloc so no data can leak appart from previous readings.

Fixes tag is beyond some major refactoring so likely manual backporting
would be needed to get that far back.

Whilst the force alignment of the ts is not strictly necessary, it
does make the code less fragile.

Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware fifo")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 24864d9dfab5..48435865fdaf 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -189,6 +189,14 @@ struct bmc150_accel_data {
 	struct mutex mutex;
 	u8 fifo_mode, watermark;
 	s16 buffer[8];
+	/*
+	 * Ensure there is sufficient space and correct alignment for
+	 * the timestamp if enabled
+	 */
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
 	u8 bw_bits;
 	u32 slope_dur;
 	u32 slope_thres;
@@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
 	 * now.
 	 */
 	for (i = 0; i < count; i++) {
-		u16 sample[8];
 		int j, bit;
 
 		j = 0;
 		for_each_set_bit(bit, indio_dev->active_scan_mask,
 				 indio_dev->masklength)
-			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
+			memcpy(&data->scan.channels[j++], &buffer[i * 3 + bit],
+			       sizeof(data->scan.channels[0]));
 
-		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
+		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+						   tstamp);
 
 		tstamp += sample_period;
 	}
-- 
2.27.0

[PATCH v3 04/27] iio:accel:mma7455: 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 u8 array on the stack   As Lars also noted
this anti pattern can involve a leak of data to userspace and that
indeed can happen here.  We close both issues by moving to
a suitable structure in the iio_priv() data with alignment
ensured by use of an explicit c structure.  This data is allocated
with kzalloc so no data can leak appart from previous readings.

The force alignment of ts is not strictly necessary in this particularly
case but does make the code less fragile.

Fixes: a84ef0d181d9 ("iio: accel: add Freescale MMA7455L/MMA7456L 3-axis accelerometer driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma7455_core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/mma7455_core.c b/drivers/iio/accel/mma7455_core.c
index 7e99bcb3398d..922bd38ff6ea 100644
--- a/drivers/iio/accel/mma7455_core.c
+++ b/drivers/iio/accel/mma7455_core.c
@@ -52,6 +52,14 @@
 
 struct mma7455_data {
 	struct regmap *regmap;
+	/*
+	 * Used to reorganize data.  Will ensure correct alignment of
+	 * the timestamp if present
+	 */
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
 };
 
 static int mma7455_drdy(struct mma7455_data *mma7455)
@@ -82,19 +90,19 @@ static irqreturn_t mma7455_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mma7455_data *mma7455 = iio_priv(indio_dev);
-	u8 buf[16]; /* 3 x 16-bit channels + padding + ts */
 	int ret;
 
 	ret = mma7455_drdy(mma7455);
 	if (ret)
 		goto done;
 
-	ret = regmap_bulk_read(mma7455->regmap, MMA7455_REG_XOUTL, buf,
-			       sizeof(__le16) * 3);
+	ret = regmap_bulk_read(mma7455->regmap, MMA7455_REG_XOUTL,
+			       mma7455->scan.channels,
+			       sizeof(mma7455->scan.channels));
 	if (ret)
 		goto done;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &mma7455->scan,
 					   iio_get_time_ns(indio_dev));
 
 done:
-- 
2.27.0

[PATCH v3 05/27] iio:gyro:itg3200: 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.
This is fixed by using an explicit c structure. As there are no
holes in the structure, there is no possiblity of data leakage
in this case.

The explicit alignment of ts is not strictly necessary but potentially
makes the code slightly less fragile.  It also removes the possibility
of this being cut and paste into another driver where the alignment
isn't already true.

Fixes: 36e0371e7764 ("iio:itg3200: Use iio_push_to_buffers_with_timestamp()")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/gyro/itg3200_buffer.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/gyro/itg3200_buffer.c b/drivers/iio/gyro/itg3200_buffer.c
index d3fbe9d86467..1c3c1bd53374 100644
--- a/drivers/iio/gyro/itg3200_buffer.c
+++ b/drivers/iio/gyro/itg3200_buffer.c
@@ -46,13 +46,20 @@ static irqreturn_t itg3200_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct itg3200 *st = iio_priv(indio_dev);
-	__be16 buf[ITG3200_SCAN_ELEMENTS + sizeof(s64)/sizeof(u16)];
-
-	int ret = itg3200_read_all_channels(st->i2c, buf);
+	/*
+	 * Ensure correct alignment and padding including for the
+	 * timestamp that may be inserted.
+	 */
+	struct {
+		__be16 buf[ITG3200_SCAN_ELEMENTS];
+		s64 ts __aligned(8);
+	} scan;
+
+	int ret = itg3200_read_all_channels(st->i2c, scan.buf);
 	if (ret < 0)
 		goto error_ret;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
 
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.27.0

[PATCH v3 06/27] iio:proximity:mb1232: 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 s16 array on the stack   As Lars also noted
this anti pattern can involve a leak of data to userspace and that
indeed can happen here.  We close both issues by moving to
a suitable structure in the iio_priv() data with alignment
ensured by use of an explicit c structure.  This data is allocated
with kzalloc so no data can leak appart from previous readings.

In this case the forced alignment of the ts is necessary to ensure
correct padding on x86_32 where the s64 would only be 4 byte aligned.

Fixes: 16b05261537e ("mb1232.c: add distance iio sensor with i2c")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Andreas Klinger <ak@it-klinger.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/proximity/mb1232.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/proximity/mb1232.c b/drivers/iio/proximity/mb1232.c
index 654564c45248..ad4b1fb2607a 100644
--- a/drivers/iio/proximity/mb1232.c
+++ b/drivers/iio/proximity/mb1232.c
@@ -40,6 +40,11 @@ struct mb1232_data {
 	 */
 	struct completion	ranging;
 	int			irqnr;
+	/* Ensure correct alignment of data to push to IIO buffer */
+	struct {
+		s16 distance;
+		s64 ts __aligned(8);
+	} scan;
 };
 
 static irqreturn_t mb1232_handle_irq(int irq, void *dev_id)
@@ -113,17 +118,13 @@ static irqreturn_t mb1232_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mb1232_data *data = iio_priv(indio_dev);
-	/*
-	 * triggered buffer
-	 * 16-bit channel + 48-bit padding + 64-bit timestamp
-	 */
-	s16 buffer[8] = { 0 };
 
-	buffer[0] = mb1232_read_distance(data);
-	if (buffer[0] < 0)
+	data->scan.distance = mb1232_read_distance(data);
+	if (data->scan.distance < 0)
 		goto err;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+					   pf->timestamp);
 
 err:
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.27.0

[PATCH v3 07/27] iio:chemical:ccs811: 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() data with alignment
explicitly requested.  This data is allocated with kzalloc so no
data can leak appart from previous readings.

The explicit alignment of ts is necessary to ensure consistent
padding for x86_32 in which the ts would otherwise be 4 byte aligned.

Fixes: 283d26917ad6 ("iio: chemical: ccs811: Add triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/chemical/ccs811.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
index 2b007e7568b2..60dd87e96f5f 100644
--- a/drivers/iio/chemical/ccs811.c
+++ b/drivers/iio/chemical/ccs811.c
@@ -78,6 +78,11 @@ struct ccs811_data {
 	struct iio_trigger *drdy_trig;
 	struct gpio_desc *wakeup_gpio;
 	bool drdy_trig_on;
+	/* Ensures correct alignment of timestamp if present */
+	struct {
+		s16 channels[2];
+		s64 ts __aligned(8);
+	} scan;
 };
 
 static const struct iio_chan_spec ccs811_channels[] = {
@@ -327,17 +332,17 @@ static irqreturn_t ccs811_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ccs811_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
-	s16 buf[8]; /* s16 eCO2 + s16 TVOC + padding + 8 byte timestamp */
 	int ret;
 
-	ret = i2c_smbus_read_i2c_block_data(client, CCS811_ALG_RESULT_DATA, 4,
-					    (u8 *)&buf);
+	ret = i2c_smbus_read_i2c_block_data(client, CCS811_ALG_RESULT_DATA,
+					    sizeof(data->scan.channels),
+					    (u8 *)data->scan.channels);
 	if (ret != 4) {
 		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:
-- 
2.27.0

[PATCH v3 08/27] 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.

Depending on the enabled channels, the  location of the timestamp
can be at various aligned offsets through the buffer.  As such we
any use of a structure to enforce this alignment would incorrectly
suggest a single location for the timestamp.

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>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 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 155faaea8c72..e8bdc221d65b 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.27.0

[PATCH v3 09/27] 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.

It is necessary to force the alignment of ts to avoid the padding
on x86_32 being different from 64 bit platorms (it alows for
4 bytes aligned 8 byte types.

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 aa8ed1e3e89a..b8e721bced5b 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 __aligned(8);
+	} 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.27.0

[PATCH v3 10/27] 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.

The forced alignment of ts is not necessary in this case but it
potentially makes the code less fragile.

Fixes: e12ffd241c00 ("iio: light: rpr0521 triggered buffer")
Cc: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 aa2972b04833..31224a33bade 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 __aligned(8);
+	} 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.27.0

[PATCH v3 11/27] 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.

A local unsigned int variable is used for the regmap call so it
is clear there is no potential issue with writing into the padding
of the structure.

Fixes: 3025c8688c1e ("iio: light: add support for UVIS25 sensor")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/light/st_uvis25.h      | 5 +++++
 drivers/iio/light/st_uvis25_core.c | 8 +++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/st_uvis25.h b/drivers/iio/light/st_uvis25.h
index 78bc56aad129..283086887caf 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 __aligned(8);
+	} 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 a18a82e6bbf5..50d9850256fc 100644
--- a/drivers/iio/light/st_uvis25_core.c
+++ b/drivers/iio/light/st_uvis25_core.c
@@ -232,17 +232,19 @@ 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);
+	unsigned int val;
 	int err;
 
-	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR, (int *)buffer);
+	err = regmap_read(hw->regmap, ST_UVIS25_REG_OUT_ADDR, &val);
 	if (err < 0)
 		goto out;
 
-	iio_push_to_buffers_with_timestamp(iio_dev, buffer,
+	hw->scan.chan = val & 0xFF;
+
+	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan,
 					   iio_get_time_ns(iio_dev));
 
 out:
-- 
2.27.0

[PATCH v3 12/27] 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.

Forced alignment of ts is not strictly necessary but probably makes
the code slightly less fragile.

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 4bac0646398d..b4323d2db0b1 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 __aligned(8);
+	} 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.27.0

[PATCH v3 13/27] 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.

The explicit alignment of ts is not necessary in this case as by
coincidence the padding will end up the same, however I consider
it to make the code less fragile and have included it.

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

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 03d71f796177..623766ff800b 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -366,6 +366,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 __aligned(8);
+	} scan;
 };
 
 /* Enable attached power regulator if any. */
@@ -793,7 +799,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);
@@ -816,12 +821,13 @@ 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);
+	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, buff,
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
 					   iio_get_time_ns(indio_dev));
+
 	return;
 
 unlock:
-- 
2.27.0

[PATCH v3 14/27] 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.

The explicit alignment of ts is not necessary in this case but
does make the code slightly less fragile so I have included it.

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 4d305a21c379..e96113ca39bd 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 __aligned(8);
+	} 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.27.0

[PATCH v3 15/27] 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.

In this driver, depending on which channels are enabled, the timestamp
can be in a number of locations.  Hence we cannot use a structure
to specify the datalayout without it being missleading.

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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 a82e040bd109..d29f1b5d1658 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -10,6 +10,8 @@ struct bmi160_data {
 	struct iio_trigger *trig;
 	struct regulator_bulk_data supplies[2];
 	struct iio_mount_matrix orientation;
+	/* 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 222ebb26f013..86cfd75ea125 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -427,7 +427,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;
@@ -438,10 +437,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.27.0

[PATCH v3 16/27] 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 an array of suitable structures in the iio_priv() data.

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

For the tagged path the data is aligned by using __aligned(8) for
the buffer on the stack.

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

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index d80ba2e688ed..9275346a9cc1 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -383,6 +383,7 @@ struct st_lsm6dsx_sensor {
  * @iio_devs: Pointers to acc/gyro iio_dev instances.
  * @settings: Pointer to the specific sensor settings in use.
  * @orientation: sensor chip orientation relative to main hardware.
+ * @scan: Temporary buffers used to align data before iio_push_to_buffers()
  */
 struct st_lsm6dsx_hw {
 	struct device *dev;
@@ -411,6 +412,11 @@ struct st_lsm6dsx_hw {
 	const struct st_lsm6dsx_settings *settings;
 
 	struct iio_mount_matrix orientation;
+	/* Ensure natural alignment of buffer elements */
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan[3];
 };
 
 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 7de10bd636ea..9e0404e3581d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -353,9 +353,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;
@@ -416,19 +413,22 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
 
 		while (acc_sip > 0 || gyro_sip > 0 || ext_sip > 0) {
 			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
-				memcpy(gyro_buff, &hw->buff[offset],
-				       ST_LSM6DSX_SAMPLE_SIZE);
-				offset += ST_LSM6DSX_SAMPLE_SIZE;
+				memcpy(hw->scan[ST_LSM6DSX_ID_GYRO].channels,
+				       &hw->buff[offset],
+				       sizeof(hw->scan[ST_LSM6DSX_ID_GYRO].channels));
+				offset += sizeof(hw->scan[ST_LSM6DSX_ID_GYRO].channels);
 			}
 			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
-				memcpy(acc_buff, &hw->buff[offset],
-				       ST_LSM6DSX_SAMPLE_SIZE);
-				offset += ST_LSM6DSX_SAMPLE_SIZE;
+				memcpy(hw->scan[ST_LSM6DSX_ID_ACC].channels,
+				       &hw->buff[offset],
+				       sizeof(hw->scan[ST_LSM6DSX_ID_ACC].channels));
+				offset += sizeof(hw->scan[ST_LSM6DSX_ID_ACC].channels);
 			}
 			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
-				memcpy(ext_buff, &hw->buff[offset],
-				       ST_LSM6DSX_SAMPLE_SIZE);
-				offset += ST_LSM6DSX_SAMPLE_SIZE;
+				memcpy(hw->scan[ST_LSM6DSX_ID_EXT0].channels,
+				       &hw->buff[offset],
+				       sizeof(hw->scan[ST_LSM6DSX_ID_EXT0].channels));
+				offset += sizeof(hw->scan[ST_LSM6DSX_ID_EXT0].channels);
 			}
 
 			if (ts_sip-- > 0) {
@@ -458,19 +458,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->scan[ST_LSM6DSX_ID_GYRO],
+					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->scan[ST_LSM6DSX_ID_ACC],
+					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->scan[ST_LSM6DSX_ID_EXT0],
+					ext_sensor->ts_ref + ts);
 				ext_sip--;
 			}
 			sip++;
@@ -555,7 +558,8 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
 {
 	u16 pattern_len = hw->sip * ST_LSM6DSX_TAGGED_SAMPLE_SIZE;
 	u16 fifo_len, fifo_diff_mask;
-	u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE], tag;
+	u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
+	u8 tag;
 	bool reset_ts = false;
 	int i, err, read_len;
 	__le16 fifo_status;
-- 
2.27.0

[PATCH v3 17/27] 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>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 +++++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 12 +++++-------
 2 files changed, 10 insertions(+), 10 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 b533fa2dad0a..d8e6b88ddffc 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,11 @@ 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,
-				  INV_MPU6050_FIFO_COUNT_BYTE);
+	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 +180,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 +189,7 @@ 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.27.0

[PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.

[Jonathan Cameron]

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

We should not be assuming that we are reading a sequence of
registers as here we are doing a read of a lot of data from
a single register address.

Suggested-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index d8e6b88ddffc..45c37525c2f1 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -179,8 +179,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	nb = fifo_count / bytes_per_datum;
 	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,
-					  st->data, bytes_per_datum);
+		result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
+					   st->data, bytes_per_datum);
 		if (result)
 			goto flush_fifo;
 		/* skip first samples if needed */
-- 
2.27.0

[PATCH v3 19/27] 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>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 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 ccdb0b70e48c..8a481dbe808c 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.27.0

[PATCH v3 20/27] 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.

The eplicit alignment of ts is necessary to ensure correct padding
on x86_32 where s64 is only aligned to 4 bytes.

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 9426f70a8005..cf63983a54d9 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 __aligned(8);
+	} 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.27.0

[PATCH v3 21/27] 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.

The force alignment of ts is not strictly necessary in this case
but reduces the fragility of the code.

Fixes: 3691e5a69449 ("iio: adc: add driver for the ti-adc084s021 chip")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Mårten Lindahl <martenli@axis.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 9017e1e24273..dfba34834a57 100644
--- a/drivers/iio/adc/ti-adc084s021.c
+++ b/drivers/iio/adc/ti-adc084s021.c
@@ -26,6 +26,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 __aligned(8);
+	} scan;
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache line.
@@ -141,14 +146,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.27.0

[PATCH v3 22/27] 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 dfba34834a57..fb14b92fa6e7 100644
--- a/drivers/iio/adc/ti-adc084s021.c
+++ b/drivers/iio/adc/ti-adc084s021.c
@@ -70,11 +70,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);
@@ -82,7 +81,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;
 }
@@ -93,6 +92,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:
@@ -107,13 +107,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.27.0

[PATCH v3 23/27] 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.

In this driver the timestamp can end up in various different locations
depending on what other channels are enabled.  As a result, we don't
use a structure to specify it's position as that would be missleading.

Fixes: e717f8c6dfec ("iio: adc: Add the TI ads124s08 ADC code")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 4b4fbe33930c..734ee5d82ff6 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.27.0

[PATCH v3 24/27] 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.

In this case the postioning of the timestamp is depends on what
other channels are enabled. As such we cannot use a structure to
make the alignment explicit as it would be missleading by suggesting
only one possible location for the timestamp.

Fixes: 815bbc87462a ("iio: ti-adc0832: add triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 c7a085dce1f4..71d49f97ab9c 100644
--- a/drivers/iio/adc/ti-adc0832.c
+++ b/drivers/iio/adc/ti-adc0832.c
@@ -29,6 +29,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];
@@ -200,7 +202,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;
 
@@ -218,10 +219,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.27.0

[PATCH v3 25/27] 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.

In this case the timestamp location depends on what other channels
are enabled. As such we can't use a structure without misleading
by suggesting only one possible timestamp location.

Fixes: 50a6edb1b6e0 ("iio: adc: add ADC12130/ADC12132/ADC12138 ADC driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 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 e485719cd2c4..16f4fd7a04d9 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.27.0

[PATCH v3 26/27] 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. The explicit alignment
isn't technically needed here, but it reduced fragility and avoids
cut and paste into drivers where it will be needed.

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>
Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Cc: Marc Titinger <mtitinger@baylibre.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ina2xx-adc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 5ed63e874292..b573ec60a8b8 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -146,6 +146,11 @@ 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 timestamp */
+	struct {
+		u16 chan[4];
+		u64 ts __aligned(8);
+	} scan;
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -738,8 +743,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 +761,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 		if (ret < 0)
 			return ret;
 
-		data[i++] = val;
+		chip->scan.chan[i++] = val;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
+	iio_push_to_buffers_with_timestamp(indio_dev, &chip->scan, time);
 
 	return 0;
 };
-- 
2.27.0

[PATCH v3 27/27] 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.

The explicit alignment of ts is necessary to ensure correct padding
on architectures where s64 is only 4 bytes aligned such as x86_32.

Fixes: a9e9c7153e96 ("iio: adc: add max1117/max1118/max1119 ADC driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/max1118.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/max1118.c b/drivers/iio/adc/max1118.c
index 01b20e420ac4..6efb0b43d938 100644
--- a/drivers/iio/adc/max1118.c
+++ b/drivers/iio/adc/max1118.c
@@ -36,6 +36,11 @@ struct max1118 {
 	struct spi_device *spi;
 	struct mutex lock;
 	struct regulator *reg;
+	/* Ensure natural alignment of buffer elements */
+	struct {
+		u8 channels[2];
+		s64 ts __aligned(8);
+	} scan;
 
 	u8 data ____cacheline_aligned;
 };
@@ -166,7 +171,6 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct max1118 *adc = iio_priv(indio_dev);
-	u8 data[16] = { }; /* 2x 8-bit ADC data + padding + 8 bytes timestamp */
 	int scan_index;
 	int i = 0;
 
@@ -184,10 +188,10 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p)
 			goto out;
 		}
 
-		data[i] = ret;
+		adc->scan.channels[i] = ret;
 		i++;
 	}
-	iio_push_to_buffers_with_timestamp(indio_dev, data,
+	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
 					   iio_get_time_ns(indio_dev));
 out:
 	mutex_unlock(&adc->lock);
-- 
2.27.0

Re: [PATCH v3 08/27] iio:light:si1145: Fix timestamp alignment and prevent data leak.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 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 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.
>
> Depending on the enabled channels, the  location of the timestamp
> can be at various aligned offsets through the buffer.  As such we
> any use of a structure to enforce this alignment would incorrectly
> suggest a single location for the timestamp.

...

> +       /* Ensure timestamp will be naturally aligned if present */
> +       u8 buffer[24] __aligned(8);

Why can't we use proper structure here?

> @@ -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];

Seems even the old comment shows how it should look like...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/27] iio:light:si1145: Fix timestamp alignment and prevent data leak.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 10:43 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jul 22, 2020 at 6:53 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 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.
> >
> > Depending on the enabled channels, the  location of the timestamp
> > can be at various aligned offsets through the buffer.  As such we
> > any use of a structure to enforce this alignment would incorrectly
> > suggest a single location for the timestamp.
>
> ...
>
> > +       /* Ensure timestamp will be naturally aligned if present */
> > +       u8 buffer[24] __aligned(8);
>
> Why can't we use proper structure here?
>
> > @@ -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];
>
> Seems even the old comment shows how it should look like...

I think I understand now. Basically it's a dynamic amount of channels
(up to 6) before you get a timestamp.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/27] iio:light:rpr0521 Fix timestamp alignment and prevent data leak.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 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().

> This data is allocated with kzalloc so no data can leak appart

apart

> from previous readings and in this case the status byte from the device.
>
> The forced alignment of ts is not necessary in this case but it
> potentially makes the code less fragile.

...

> +        * Note that the read will put garbage data into
> +        * the padding but this should not be a problem

> +               u8 garbage;

>         err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> -               &buffer,
> +               data->scan.channels,
>                 (3 * 2) + 1);   /* 3 * 16-bit + (discarded) int clear reg. */

But can't we read the interrupt clear register separately?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/27] iio:light:st_uvis25 Fix timestamp alignment and prevent data leak.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 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()
>
> This data is allocated with kzalloc so no data can leak apart
> from previous readings.
>
> A local unsigned int variable is used for the regmap call so it
> is clear there is no potential issue with writing into the padding
> of the structure.

...

> +               u8 chan;

> +       hw->scan.chan = val & 0xFF;

0xFF is not needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 23/27] iio:adc:ti-ads124s08 Fix alignment and data leak issues.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 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 with alignment
> explicitly requested.  This data is allocated with kzalloc so no
> data can leak apart from previous readings.
>
> In this driver the timestamp can end up in various different locations
> depending on what other channels are enabled.  As a result, we don't
> use a structure to specify it's position as that would be missleading.

...

> +       /*
> +        * Used to correctly align data.
> +        * Ensure timestamp is naturally aligned.
> +        */
> +       u32 buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);

u32 vs. u16?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 23/27] iio:adc:ti-ads124s08 Fix alignment and data leak issues.

[Jonathan Cameron]

On Wed, 22 Jul 2020 23:54:29 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 22, 2020 at 6:53 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 with alignment
> > explicitly requested.  This data is allocated with kzalloc so no
> > data can leak apart from previous readings.
> >
> > In this driver the timestamp can end up in various different locations
> > depending on what other channels are enabled.  As a result, we don't
> > use a structure to specify it's position as that would be missleading.  
> 
> ...
> 
> > +       /*
> > +        * Used to correctly align data.
> > +        * Ensure timestamp is naturally aligned.
> > +        */
> > +       u32 buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);  
> 
> u32 vs. u16?
> 
Curious indeed.  My eyes jumped straight over that when cutting and
pasting that line.  I guess too big is never a problem, but should definitely
tidy that up.

Thanks,

Jonathan

Re: [PATCH v3 08/27] iio:light:si1145: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 22:45:59 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 22, 2020 at 10:43 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Jul 22, 2020 at 6:53 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 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.
> > >
> > > Depending on the enabled channels, the  location of the timestamp
> > > can be at various aligned offsets through the buffer.  As such we
> > > any use of a structure to enforce this alignment would incorrectly
> > > suggest a single location for the timestamp.  
> >
> > ...
> >  
> > > +       /* Ensure timestamp will be naturally aligned if present */
> > > +       u8 buffer[24] __aligned(8);  
> >
> > Why can't we use proper structure here?
> >  
> > > @@ -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];  
> >
> > Seems even the old comment shows how it should look like...  
> 
> I think I understand now. Basically it's a dynamic amount of channels
> (up to 6) before you get a timestamp.
> 
Exactly.  Comment is giving the largest it can be, not what is needed for
a given configuration of the device.

Should indeed drop that comment.  Obviously went into automated mode and stopped
actually reading what was in front of me.

Jonathan

Re: [PATCH v3 10/27] iio:light:rpr0521 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 22:47:46 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 22, 2020 at 6:53 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().  
> 
> > This data is allocated with kzalloc so no data can leak appart  
> 
> apart
> 
> > from previous readings and in this case the status byte from the device.
> >
> > The forced alignment of ts is not necessary in this case but it
> > potentially makes the code less fragile.  
> 
> ...
> 
> > +        * Note that the read will put garbage data into
> > +        * the padding but this should not be a problem  
> 
> > +               u8 garbage;  
> 
> >         err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> > -               &buffer,
> > +               data->scan.channels,
> >                 (3 * 2) + 1);   /* 3 * 16-bit + (discarded) int clear reg. */  
> 
> But can't we read the interrupt clear register separately?
> 

Potentially though I have no idea if there is an odd quirk there. I'm not
immediately seeing anything on the datasheet about it.
Would need a tested-by from someone with hardware to confirm it is fine to
do it as two reads though.

Would be nice to get rid of the non standard handling so well worth
pursuing in v4 of this set.

Jonathan

Re: [PATCH v3 14/27] iio:magnetometer:mag3110 Fix alignment and data leak issues.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 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 apart from
> previous readings.
>
> The explicit alignment of ts is not necessary in this case but
> does make the code slightly less fragile so I have included it.
>

I broke my eyes and brain over temp.
At first I thought it was temporary garbage like in the other patch,
but now I'm guessing (looking into the comment) that this is
temperature.
Can we spell it fully?

...

> +               u8 temp;

> -       u8 buffer[16]; /* 3 16-bit channels + 1 byte temp + padding + ts */

> +               data->scan.temp = ret;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> We should not be assuming that we are reading a sequence of
> registers as here we are doing a read of a lot of data from
> a single register address.

> -               result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> -                                         st->data, bytes_per_datum);
> +               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
> +                                          st->data, bytes_per_datum);

I don't know the difference between these APIs, but AFAIU in this case
we always ask for a minimum data (like one item of 2 bytes or so) per
access. Because registers are defined like 16-bit wide we are fine. Is
that correct?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 19/27] iio:pressure:mpl3115 Force alignment of buffer

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <jic23@kernel.org> wrote:

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

I guess all non-standard cases (where no struct is applicable) deserve
for better comment in the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/27] IIO: Fused set 1 and 2 of timestamp alignment fixes

[Andy Shevchenko]

On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> These are back again. Please take a look at them.  I'm fairly sure these
> are all 'safe' but really don't like taking patches without the surety of
> a second pair of eyes having taken a look at them. That is particularly
> true of fixes for long term issues like these ones.
>

So, for all non-commented
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
with a condition that you amend comments in the code. I see that they
can be categorized into three categories:
1. struct approach (no [big] comment needed, we have __aligned self-explanatory)
2. dynamic channels (nothing specifically needed, perhaps mention "the
amount of channels is defined at run-time" or alike)
3. the rest of non-standard cases where per case comment is needed.

Above applies to a subset of commented patches (where my comment about
the topic).

The rest I will look at v4.

Thanks!

> Changes since v2:
> * bmc150-accel: Use sizeof() for channel size (Andy Shevchenko)
> * st_uvis25: Use local variable for regmap call (Andy Shevchenko)
> * st_lsm6dsx: Use array of scan[] rather than 3 structures (Lorenzo Bianconi)
> * inv_mpu6050: Add patch switching to a regmap_noinc_read (Jean-Baptiste Maneyrol)
> * ina2xx: Use a structure (previously failed to notice that works here)
> * I've added clarifying notes to patch descriptions based on questions asked.
>   These were mainly about why we didn't use the structure approach everywhere
>   and why I've forced alignment in places it wasn't strictly needed.
>
> Previous cover letter:
> A few notes based on questions on v1.
>
> 1. Why not use put_unaligned to avoid the whole thing?
>    This interface can pass directly to a variety of in kernel users
>    who would reasonably assume that we have true natural alignment.
>    When it gets passed directly is subtle as it depends on whether
>    the demux has to realign the data or not.  So enabling an extra
>    channel could result in a previously working alignment no longer
>    being true.
>
>    Even if this is fine for existing usecases we are likely to
>    store up subtle long term issues if we don't fix it explicitly.
>    It's also worth noting that the data channel sometimes suffered
>    the same problem as the timestamp.
>
> 2. Why not specify explicit padding?
>    In my view this is error prone in comparisom with relying on
>    c to do the hard work for us.
>
> 3. Why not move the timestamp to the start?
>    ABI breakage and as timestamp is optional (no obvious from the
>    iio_push_to_buffers_with_timestamp call) we can end up having
>    to shift the rest of the data within that call.
>
> Changes since v1.
>
> Andy Schevchenko pointed out that on x86_32 s64 elements are only
> aligned to 4 bytes.  Where I had tried to use a structure to avoid
> explicit need to list the padding, there were some cases where
> this results in insufficient padding being inserted.
>
> This doesn't affect the few patches that had already been applied and
> sent upstream. (which was lucky ;)
>
> The fix was to take advantage of __aligned(8) which (according to
> my reading of the c spec and the gcc docs) enforces the alignment of
> both the element within a structure and the structure itself.
> The kernel now requires a recent enough version of GCC to ensure this
> works both on the stack and heap.  This is done in lots of other
> userspace interfaces. In some cases iio_push_to_buffers_with_ts
> is aligning data for passing to userspace, be it via a kfifo
> so it is sensible we should use the same solution.
>
> Note that we could have used u64_aligned but there is no equivalent
> for s64 and explicit use of __aligned(8) is common in
> the kernel so we adopt this here.
>
> Note that there were about 8 drivers that would have been broken with
> v1 of the patch.  I have also forced alignment of timestamps in cases
> where (mostly by coincidence) we would have been fine (padding was
> less than 4 bytes anyway.  I did this partly to reduce fragility if
> other elements are added in future and also to avoid cut and paste
> errors in new drivers.
>
> There were a few other minor tidying up changes inline with reviews
> of v1.
>
> I've kept tags given for v1 on basis the changes are minor. Shout if
> you disagree.
>
> Version 1 part 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 (27):
>   iio: accel: kxsd9: Fix alignment of local buffer.
>   iio:accel:mma8452: Fix timestamp alignment and prevent data leak.
>   iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak.
>   iio:accel:mma7455: Fix timestamp alignment and prevent data leak.
>   iio:gyro:itg3200: Fix timestamp alignment and prevent data leak.
>   iio:proximity:mb1232: Fix timestamp alignment and prevent data leak.
>   iio:chemical:ccs811: Fix timestamp alignment and prevent data leak.
>   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:ak8975 Fix alignment and data leak issues.
>   iio:magnetometer:mag3110 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:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.
>   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-ads124s08 Fix alignment and data leak issues.
>   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/accel/bmc150-accel-core.c         | 15 ++++++--
>  drivers/iio/accel/kxsd9.c                     | 16 ++++++---
>  drivers/iio/accel/mma7455_core.c              | 16 ++++++---
>  drivers/iio/accel/mma8452.c                   | 11 ++++--
>  drivers/iio/adc/ina2xx-adc.c                  | 11 +++---
>  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-ads124s08.c                | 10 ++++--
>  drivers/iio/chemical/ccs811.c                 | 13 ++++---
>  drivers/iio/gyro/itg3200_buffer.c             | 15 +++++---
>  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    | 14 ++++----
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  6 ++++
>  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 36 ++++++++++---------
>  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            |  8 +++--
>  drivers/iio/magnetometer/ak8975.c             | 16 ++++++---
>  drivers/iio/magnetometer/mag3110.c            | 13 ++++---
>  drivers/iio/pressure/mpl3115.c                |  3 +-
>  drivers/iio/proximity/mb1232.c                | 17 ++++-----
>  29 files changed, 229 insertions(+), 119 deletions(-)
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.

[Jonathan Cameron]

On Thu, 23 Jul 2020 15:15:46 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > We should not be assuming that we are reading a sequence of
> > registers as here we are doing a read of a lot of data from
> > a single register address.  
> 
> > -               result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> > -                                         st->data, bytes_per_datum);
> > +               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
> > +                                          st->data, bytes_per_datum);  
> 
> I don't know the difference between these APIs, but AFAIU in this case
> we always ask for a minimum data (like one item of 2 bytes or so) per
> access. Because registers are defined like 16-bit wide we are fine. Is
> that correct?
Yes.   There is only really a different in these two APIs if caching
is enabled in regmap.  Conceptually noinc is repeated reading of the same
register, whilst build_read reads a bunch of registers starting at this
location.   If any of them happen to be cacheable, regmap_bulk_read will
update the cached value for any registers it happens to read.  In this
particular case that would be incorrect because the hardware is not
doing any auto increment of the address during repeated reads, it's just
reading the same register lots of times.

Jonathan

Re: [PATCH v3 19/27] iio:pressure:mpl3115 Force alignment of buffer

[Jonathan Cameron]

On Thu, 23 Jul 2020 15:17:16 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > 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.  
> 
> I guess all non-standard cases (where no struct is applicable) deserve
> for better comment in the code.
> 

Sure can do that. I guess that will mean people actually think about
it whilst copying examples form old into new drivers.

In many ways the no struct applicable is the 'standard' case. It's
mere coincidence that in some drivers there are sufficiently few
channels that the struct 'trick' works.  All the other channels
are moving around even in those cases, it's just the timestamp
that happens to only have one valid location.

I'll probably do a v4 at the weekend.

Jonathan

Re: [PATCH v3 17/27] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.

[Jean-Baptiste Maneyrol]

Hi Jonathan,

looks perfect for me.

Thanks for the fix,
JB

Reviewed-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>


From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Cameron <jic23@kernel.org>
Sent: Wednesday, July 22, 2020 17:50
To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald <pmeerw@pmeerw.net>; Jonathan Cameron <Jonathan.Cameron@huawei.com>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
Subject: [PATCH v3 17/27] 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>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 +++++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 12 +++++-------
 2 files changed, 10 insertions(+), 10 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 b533fa2dad0a..d8e6b88ddffc 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,11 @@ 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,
-                                 INV_MPU6050_FIFO_COUNT_BYTE);
+       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 +180,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 +189,7 @@ 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.27.0

Re: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.

[Jean-Baptiste Maneyrol]

Hi Jonathan,

perfect.

Thanks for the fix,
JB

Reviewed-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>


From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Cameron <jic23@kernel.org>
Sent: Wednesday, July 22, 2020 17:50
To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald <pmeerw@pmeerw.net>; Jonathan Cameron <Jonathan.Cameron@huawei.com>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
Subject: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads. 
 
 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>

We should not be assuming that we are reading a sequence of
registers as here we are doing a read of a lot of data from
a single register address.

Suggested-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index d8e6b88ddffc..45c37525c2f1 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -179,8 +179,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
         nb = fifo_count / bytes_per_datum;
         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,
-                                         st->data, bytes_per_datum);
+               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
+                                          st->data, bytes_per_datum);
                 if (result)
                         goto flush_fifo;
                 /* skip first samples if needed */
-- 
2.27.0

Re: [PATCH v3 03/27] iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak.

[Srinivas Pandruvada]

On Wed, 2020-07-22 at 16:50 +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses a 16 byte array of smaller elements on the
> stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving
> to a suitable structure in the iio_priv() data with alignment
> ensured by use of an explicit c structure.  This data is allocated
> with kzalloc so no data can leak appart from previous readings.
> 
> Fixes tag is beyond some major refactoring so likely manual
> backporting
> would be needed to get that far back.
> 
> Whilst the force alignment of the ts is not strictly necessary, it
> does make the code less fragile.
> 
> Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware
> fifo")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c
> b/drivers/iio/accel/bmc150-accel-core.c
> index 24864d9dfab5..48435865fdaf 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -189,6 +189,14 @@ struct bmc150_accel_data {
>  	struct mutex mutex;
>  	u8 fifo_mode, watermark;
>  	s16 buffer[8];
> +	/*
> +	 * Ensure there is sufficient space and correct alignment for
> +	 * the timestamp if enabled
> +	 */
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
>  	u8 bw_bits;
>  	u32 slope_dur;
>  	u32 slope_thres;
> @@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct
> iio_dev *indio_dev,
>  	 * now.
>  	 */
>  	for (i = 0; i < count; i++) {
> -		u16 sample[8];
>  		int j, bit;
>  
>  		j = 0;
>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
>  				 indio_dev->masklength)
> -			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
> +			memcpy(&data->scan.channels[j++], &buffer[i * 3
> + bit],
> +			       sizeof(data->scan.channels[0]));
>  
> -		iio_push_to_buffers_with_timestamp(indio_dev, sample,
> tstamp);
> +		iio_push_to_buffers_with_timestamp(indio_dev, &data-
> >scan,
> +						   tstamp);
>  
>  		tstamp += sample_period;
>  	}

Re: [PATCH v3 01/27] iio: accel: kxsd9: Fix alignment of local buffer.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:37 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> iio_push_to_buffers_with_timestamp assumes 8 byte alignment which
> is not guaranteed by an array of smaller elements.
> 
> Note that whilst in this particular case the alignment forcing
> of the ts element is not strictly necessary it acts as good
> documentation.  Doing this where not necessary should cut
> down on the number of cut and paste introduced errors elsewhere.
> 
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan



> ---
>  drivers/iio/accel/kxsd9.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index 66b2e4cf24cf..0e18b92e2099 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -209,14 +209,20 @@ static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
>  	const struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct kxsd9_state *st = iio_priv(indio_dev);
> +	/*
> +	 * Ensure correct positioning and alignment of timestamp.
> +	 * No need to zero initialize as all elements written.
> +	 */
> +	struct {
> +		__be16 chan[4];
> +		s64 ts __aligned(8);
> +	} hw_values;
>  	int ret;
> -	/* 4 * 16bit values AND timestamp */
> -	__be16 hw_values[8];
>  
>  	ret = regmap_bulk_read(st->map,
>  			       KXSD9_REG_X,
> -			       &hw_values,
> -			       8);
> +			       hw_values.chan,
> +			       sizeof(hw_values.chan));
>  	if (ret) {
>  		dev_err(st->dev,
>  			"error reading data\n");
> @@ -224,7 +230,7 @@ static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
>  	}
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev,
> -					   hw_values,
> +					   &hw_values,
>  					   iio_get_time_ns(indio_dev));
>  	iio_trigger_notify_done(indio_dev->trig);
>  

Re: [PATCH v3 02/27] iio:accel:mma8452: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses a 16 byte u8 array on the stack.  As Lars also noted
> this anti pattern can involve a leak of data to userspace and that
> indeed can happen here.  We close both issues by moving to
> a suitable structure in the iio_priv() data with alignment
> ensured by use of an explicit c structure.  This data is allocated
> with kzalloc so no data can leak appart from previous readings.
> 
> The additional forcing of the 8 byte alignment of the timestamp
> is not strictly necessary but makes the code less fragile by
> making this explicit.
> 
> Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/mma8452.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index ba27f8673131..1cf2b5db26ca 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -110,6 +110,12 @@ struct mma8452_data {
>  	int sleep_val;
>  	struct regulator *vdd_reg;
>  	struct regulator *vddio_reg;
> +
> +	/* Ensure correct alignment of time stamp when present */
> +	struct {
> +		__be16 channels[3];
> +		s64 ts __aligned(8);
> +	} buffer;
>  };
>  
>   /**
> @@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>  	int ret;
>  
> -	ret = mma8452_read(data, (__be16 *)buffer);
> +	ret = mma8452_read(data, data->buffer.channels);
>  	if (ret < 0)
>  		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:

Re: [PATCH v3 03/27] iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 29 Jul 2020 10:12:36 -0700
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> On Wed, 2020-07-22 at 16:50 +0100, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > One of a class of bugs pointed out by Lars in a recent review.
> > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> > to the size of the timestamp (8 bytes).  This is not guaranteed in
> > this driver which uses a 16 byte array of smaller elements on the
> > stack.
> > As Lars also noted this anti pattern can involve a leak of data to
> > userspace and that indeed can happen here.  We close both issues by
> > moving
> > to a suitable structure in the iio_priv() data with alignment
> > ensured by use of an explicit c structure.  This data is allocated
> > with kzalloc so no data can leak appart from previous readings.
> > 
> > Fixes tag is beyond some major refactoring so likely manual
> > backporting
> > would be needed to get that far back.
> > 
> > Whilst the force alignment of the ts is not strictly necessary, it
> > does make the code less fragile.
> > 
> > Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware
> > fifo")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel-core.c
> > b/drivers/iio/accel/bmc150-accel-core.c
> > index 24864d9dfab5..48435865fdaf 100644
> > --- a/drivers/iio/accel/bmc150-accel-core.c
> > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > @@ -189,6 +189,14 @@ struct bmc150_accel_data {
> >  	struct mutex mutex;
> >  	u8 fifo_mode, watermark;
> >  	s16 buffer[8];
> > +	/*
> > +	 * Ensure there is sufficient space and correct alignment for
> > +	 * the timestamp if enabled
> > +	 */
> > +	struct {
> > +		__le16 channels[3];
> > +		s64 ts __aligned(8);
> > +	} scan;
> >  	u8 bw_bits;
> >  	u32 slope_dur;
> >  	u32 slope_thres;
> > @@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct
> > iio_dev *indio_dev,
> >  	 * now.
> >  	 */
> >  	for (i = 0; i < count; i++) {
> > -		u16 sample[8];
> >  		int j, bit;
> >  
> >  		j = 0;
> >  		for_each_set_bit(bit, indio_dev->active_scan_mask,
> >  				 indio_dev->masklength)
> > -			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
> > +			memcpy(&data->scan.channels[j++], &buffer[i * 3
> > + bit],
> > +			       sizeof(data->scan.channels[0]));
> >  
> > -		iio_push_to_buffers_with_timestamp(indio_dev, sample,
> > tstamp);
> > +		iio_push_to_buffers_with_timestamp(indio_dev, &data-  
> > >scan,  
> > +						   tstamp);
> >  
> >  		tstamp += sample_period;
> >  	}  
> 

Re: [PATCH v3 04/27] iio:accel:mma7455: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:40 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses a 16 byte u8 array on the stack   As Lars also noted
> this anti pattern can involve a leak of data to userspace and that
> indeed can happen here.  We close both issues by moving to
> a suitable structure in the iio_priv() data with alignment
> ensured by use of an explicit c structure.  This data is allocated
> with kzalloc so no data can leak appart from previous readings.
> 
> The force alignment of ts is not strictly necessary in this particularly
> case but does make the code less fragile.
> 
> Fixes: a84ef0d181d9 ("iio: accel: add Freescale MMA7455L/MMA7456L 3-axis accelerometer driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/mma7455_core.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma7455_core.c b/drivers/iio/accel/mma7455_core.c
> index 7e99bcb3398d..922bd38ff6ea 100644
> --- a/drivers/iio/accel/mma7455_core.c
> +++ b/drivers/iio/accel/mma7455_core.c
> @@ -52,6 +52,14 @@
>  
>  struct mma7455_data {
>  	struct regmap *regmap;
> +	/*
> +	 * Used to reorganize data.  Will ensure correct alignment of
> +	 * the timestamp if present
> +	 */
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
>  };
>  
>  static int mma7455_drdy(struct mma7455_data *mma7455)
> @@ -82,19 +90,19 @@ static irqreturn_t mma7455_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct mma7455_data *mma7455 = iio_priv(indio_dev);
> -	u8 buf[16]; /* 3 x 16-bit channels + padding + ts */
>  	int ret;
>  
>  	ret = mma7455_drdy(mma7455);
>  	if (ret)
>  		goto done;
>  
> -	ret = regmap_bulk_read(mma7455->regmap, MMA7455_REG_XOUTL, buf,
> -			       sizeof(__le16) * 3);
> +	ret = regmap_bulk_read(mma7455->regmap, MMA7455_REG_XOUTL,
> +			       mma7455->scan.channels,
> +			       sizeof(mma7455->scan.channels));
>  	if (ret)
>  		goto done;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &mma7455->scan,
>  					   iio_get_time_ns(indio_dev));
>  
>  done:

Re: [PATCH v3 06/27] iio:proximity:mb1232: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:42 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses a 16 byte s16 array on the stack   As Lars also noted
> this anti pattern can involve a leak of data to userspace and that
> indeed can happen here.  We close both issues by moving to
> a suitable structure in the iio_priv() data with alignment
> ensured by use of an explicit c structure.  This data is allocated
> with kzalloc so no data can leak appart from previous readings.
> 
> In this case the forced alignment of the ts is necessary to ensure
> correct padding on x86_32 where the s64 would only be 4 byte aligned.
> 
> Fixes: 16b05261537e ("mb1232.c: add distance iio sensor with i2c")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Andreas Klinger <ak@it-klinger.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied to the fixes-togreg branch of iio.git and marked
for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/proximity/mb1232.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/proximity/mb1232.c b/drivers/iio/proximity/mb1232.c
> index 654564c45248..ad4b1fb2607a 100644
> --- a/drivers/iio/proximity/mb1232.c
> +++ b/drivers/iio/proximity/mb1232.c
> @@ -40,6 +40,11 @@ struct mb1232_data {
>  	 */
>  	struct completion	ranging;
>  	int			irqnr;
> +	/* Ensure correct alignment of data to push to IIO buffer */
> +	struct {
> +		s16 distance;
> +		s64 ts __aligned(8);
> +	} scan;
>  };
>  
>  static irqreturn_t mb1232_handle_irq(int irq, void *dev_id)
> @@ -113,17 +118,13 @@ static irqreturn_t mb1232_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct mb1232_data *data = iio_priv(indio_dev);
> -	/*
> -	 * triggered buffer
> -	 * 16-bit channel + 48-bit padding + 64-bit timestamp
> -	 */
> -	s16 buffer[8] = { 0 };
>  
> -	buffer[0] = mb1232_read_distance(data);
> -	if (buffer[0] < 0)
> +	data->scan.distance = mb1232_read_distance(data);
> +	if (data->scan.distance < 0)
>  		goto err;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +					   pf->timestamp);
>  
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);

Re: [PATCH v3 07/27] iio:chemical:ccs811: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:43 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to a suitable structure in the iio_priv() data with alignment
> explicitly requested.  This data is allocated with kzalloc so no
> data can leak appart from previous readings.
> 
> The explicit alignment of ts is necessary to ensure consistent
> padding for x86_32 in which the ts would otherwise be 4 byte aligned.
> 
> Fixes: 283d26917ad6 ("iio: chemical: ccs811: Add triggered buffer support")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied
> ---
>  drivers/iio/chemical/ccs811.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
> index 2b007e7568b2..60dd87e96f5f 100644
> --- a/drivers/iio/chemical/ccs811.c
> +++ b/drivers/iio/chemical/ccs811.c
> @@ -78,6 +78,11 @@ struct ccs811_data {
>  	struct iio_trigger *drdy_trig;
>  	struct gpio_desc *wakeup_gpio;
>  	bool drdy_trig_on;
> +	/* Ensures correct alignment of timestamp if present */
> +	struct {
> +		s16 channels[2];
> +		s64 ts __aligned(8);
> +	} scan;
>  };
>  
>  static const struct iio_chan_spec ccs811_channels[] = {
> @@ -327,17 +332,17 @@ static irqreturn_t ccs811_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ccs811_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
> -	s16 buf[8]; /* s16 eCO2 + s16 TVOC + padding + 8 byte timestamp */
>  	int ret;
>  
> -	ret = i2c_smbus_read_i2c_block_data(client, CCS811_ALG_RESULT_DATA, 4,
> -					    (u8 *)&buf);
> +	ret = i2c_smbus_read_i2c_block_data(client, CCS811_ALG_RESULT_DATA,
> +					    sizeof(data->scan.channels),
> +					    (u8 *)data->scan.channels);
>  	if (ret != 4) {
>  		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:

Re: [PATCH v3 09/27] iio:light:max44000 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:45 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses 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.
> 
> It is necessary to force the alignment of ts to avoid the padding
> on x86_32 being different from 64 bit platorms (it alows for
> 4 bytes aligned 8 byte types.
> 
> Fixes: 06ad7ea10e2b ("max44000: Initial triggered buffer support")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable

Thanks,

Jonathan

> ---
>  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 aa8ed1e3e89a..b8e721bced5b 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 __aligned(8);
> +	} 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;

Re: [PATCH v3 12/27] iio:light:ltr501 Fix timestamp alignment issue.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:48 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> Here we use a structure on the stack.  The driver already did an
> explicit memset so no data leak was possible.
> 
> Forced alignment of ts is not strictly necessary but probably makes
> the code slightly less fragile.
> 
> 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>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  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 4bac0646398d..b4323d2db0b1 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 __aligned(8);
> +	} 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:

Re: [PATCH v3 13/27] iio:magnetometer:ak8975 Fix alignment and data leak issues.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:49 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to a suitable structure in the iio_priv() data.
> 
> This data is allocated with kzalloc so no data can leak apart from
> previous readings.
> 
> The explicit alignment of ts is not necessary in this case as by
> coincidence the padding will end up the same, however I consider
> it to make the code less fragile and have included it.
> 
> Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Gregor Boirie <gregor.boirie@parrot.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/magnetometer/ak8975.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 03d71f796177..623766ff800b 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -366,6 +366,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 __aligned(8);
> +	} scan;
>  };
>  
>  /* Enable attached power regulator if any. */
> @@ -793,7 +799,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);
> @@ -816,12 +821,13 @@ 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);
> +	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, buff,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
>  					   iio_get_time_ns(indio_dev));
> +
>  	return;
>  
>  unlock:

Re: [PATCH v3 20/27] iio:adc:ti-adc081c Fix alignment and data leak issues

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:56 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to a suitable structure in the iio_priv().
> 
> This data is allocated with kzalloc so no data can leak apart
> from previous readings.
> 
> The eplicit alignment of ts is necessary to ensure correct padding
> on x86_32 where s64 is only aligned to 4 bytes.
> 
> 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>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  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 9426f70a8005..cf63983a54d9 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 __aligned(8);
> +	} 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);

Re: [PATCH v3 21/27] iio:adc:ti-adc084s021 Fix alignment and data leak issues.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:57 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to a suitable structure in the iio_priv().
> 
> This data is allocated with kzalloc so no data can leak apart from
> previous readings.
> 
> The force alignment of ts is not strictly necessary in this case
> but reduces the fragility of the code.
> 
> Fixes: 3691e5a69449 ("iio: adc: add driver for the ti-adc084s021 chip")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Mårten Lindahl <martenli@axis.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable.

thanks,

Jonathan

> ---
>  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 9017e1e24273..dfba34834a57 100644
> --- a/drivers/iio/adc/ti-adc084s021.c
> +++ b/drivers/iio/adc/ti-adc084s021.c
> @@ -26,6 +26,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 __aligned(8);
> +	} scan;
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache line.
> @@ -141,14 +146,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);

Re: [PATCH v3 26/27] iio:adc:ina2xx Fix timestamp alignment issue.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:51:02 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses 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. The explicit alignment
> isn't technically needed here, but it reduced fragility and avoids
> cut and paste into drivers where it will be needed.
> 
> 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>
> Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> Cc: Marc Titinger <mtitinger@baylibre.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ina2xx-adc.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 5ed63e874292..b573ec60a8b8 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -146,6 +146,11 @@ 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 timestamp */
> +	struct {
> +		u16 chan[4];
> +		u64 ts __aligned(8);
> +	} scan;
>  };
>  
>  static const struct ina2xx_config ina2xx_config[] = {
> @@ -738,8 +743,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 +761,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  		if (ret < 0)
>  			return ret;
>  
> -		data[i++] = val;
> +		chip->scan.chan[i++] = val;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
> +	iio_push_to_buffers_with_timestamp(indio_dev, &chip->scan, time);
>  
>  	return 0;
>  };

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

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:51:03 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to a suitable structure in the iio_priv() data.
> 
> This data is allocated with kzalloc so no data can leak apart
> from previous readings.
> 
> The explicit alignment of ts is necessary to ensure correct padding
> on architectures where s64 is only 4 bytes aligned such as x86_32.
> 
> Fixes: a9e9c7153e96 ("iio: adc: add max1117/max1118/max1119 ADC driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/max1118.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1118.c b/drivers/iio/adc/max1118.c
> index 01b20e420ac4..6efb0b43d938 100644
> --- a/drivers/iio/adc/max1118.c
> +++ b/drivers/iio/adc/max1118.c
> @@ -36,6 +36,11 @@ struct max1118 {
>  	struct spi_device *spi;
>  	struct mutex lock;
>  	struct regulator *reg;
> +	/* Ensure natural alignment of buffer elements */
> +	struct {
> +		u8 channels[2];
> +		s64 ts __aligned(8);
> +	} scan;
>  
>  	u8 data ____cacheline_aligned;
>  };
> @@ -166,7 +171,6 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct max1118 *adc = iio_priv(indio_dev);
> -	u8 data[16] = { }; /* 2x 8-bit ADC data + padding + 8 bytes timestamp */
>  	int scan_index;
>  	int i = 0;
>  
> @@ -184,10 +188,10 @@ static irqreturn_t max1118_trigger_handler(int irq, void *p)
>  			goto out;
>  		}
>  
> -		data[i] = ret;
> +		adc->scan.channels[i] = ret;
>  		i++;
>  	}
> -	iio_push_to_buffers_with_timestamp(indio_dev, data,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
>  					   iio_get_time_ns(indio_dev));
>  out:
>  	mutex_unlock(&adc->lock);

Re: [PATCH v3 22/27] iio:adc:ti-adc084s021 Tidy up endian types

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:58 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

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

I'm a bit in 2 minds about this one.  The exact warning will change as the
result of the previous patch, but the problem is not introduced by that.
Technically it's not a 'fix' so I'll hold this one for now.

Jonathan

> ---
>  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 dfba34834a57..fb14b92fa6e7 100644
> --- a/drivers/iio/adc/ti-adc084s021.c
> +++ b/drivers/iio/adc/ti-adc084s021.c
> @@ -70,11 +70,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);
> @@ -82,7 +81,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;
>  }
> @@ -93,6 +92,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:
> @@ -107,13 +107,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;

Re: [PATCH v3 22/27] iio:adc:ti-adc084s021 Tidy up endian types

[Jonathan Cameron]

On Sun, 9 Aug 2020 18:47:55 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 22 Jul 2020 16:50:58 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > 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>  
> 
> I'm a bit in 2 minds about this one.  The exact warning will change as the
> result of the previous patch, but the problem is not introduced by that.
> Technically it's not a 'fix' so I'll hold this one for now.

Applied this one to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> 
> Jonathan
> 
> > ---
> >  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 dfba34834a57..fb14b92fa6e7 100644
> > --- a/drivers/iio/adc/ti-adc084s021.c
> > +++ b/drivers/iio/adc/ti-adc084s021.c
> > @@ -70,11 +70,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);
> > @@ -82,7 +81,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;
> >  }
> > @@ -93,6 +92,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:
> > @@ -107,13 +107,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;  
> 

Re: [PATCH v3 22/27] iio:adc:ti-adc084s021 Tidy up endian types

[Jonathan Cameron]

On Sun, 13 Sep 2020 13:01:46 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 9 Aug 2020 18:47:55 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Wed, 22 Jul 2020 16:50:58 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > 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>    
> > 
> > I'm a bit in 2 minds about this one.  The exact warning will change as the
> > result of the previous patch, but the problem is not introduced by that.
> > Technically it's not a 'fix' so I'll hold this one for now.  
> 
> Applied this one to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
And failed my local tests. Precursor not in my branch yet.
oops.
Dropped again for now.
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > Jonathan
> >   
> > > ---
> > >  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 dfba34834a57..fb14b92fa6e7 100644
> > > --- a/drivers/iio/adc/ti-adc084s021.c
> > > +++ b/drivers/iio/adc/ti-adc084s021.c
> > > @@ -70,11 +70,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);
> > > @@ -82,7 +81,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;
> > >  }
> > > @@ -93,6 +92,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:
> > > @@ -107,13 +107,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;    
> >   
> 

Re: [PATCH v3 05/27] iio:gyro:itg3200: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:41 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses a 16 byte array of smaller elements on the stack.
> This is fixed by using an explicit c structure. As there are no
> holes in the structure, there is no possiblity of data leakage
> in this case.
> 
> The explicit alignment of ts is not strictly necessary but potentially
> makes the code slightly less fragile.  It also removes the possibility
> of this being cut and paste into another driver where the alignment
> isn't already true.
> 
> Fixes: 36e0371e7764 ("iio:itg3200: Use iio_push_to_buffers_with_timestamp()")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
No idea why I didn't pick this one up with the other similar cases as
it fits in Andy's case 1 (no change needed).

Anyhow, now applied to the togreg branch of iio.git as no particular
rush to get this in.

Cc'd stable.

> ---
>  drivers/iio/gyro/itg3200_buffer.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/gyro/itg3200_buffer.c b/drivers/iio/gyro/itg3200_buffer.c
> index d3fbe9d86467..1c3c1bd53374 100644
> --- a/drivers/iio/gyro/itg3200_buffer.c
> +++ b/drivers/iio/gyro/itg3200_buffer.c
> @@ -46,13 +46,20 @@ static irqreturn_t itg3200_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct itg3200 *st = iio_priv(indio_dev);
> -	__be16 buf[ITG3200_SCAN_ELEMENTS + sizeof(s64)/sizeof(u16)];
> -
> -	int ret = itg3200_read_all_channels(st->i2c, buf);
> +	/*
> +	 * Ensure correct alignment and padding including for the
> +	 * timestamp that may be inserted.
> +	 */
> +	struct {
> +		__be16 buf[ITG3200_SCAN_ELEMENTS];
> +		s64 ts __aligned(8);
> +	} scan;
> +
> +	int ret = itg3200_read_all_channels(st->i2c, scan.buf);
>  	if (ret < 0)
>  		goto error_ret;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf, pf->timestamp);
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
>  
>  	iio_trigger_notify_done(indio_dev->trig);
>  

Re: [PATCH v3 08/27] iio:light:si1145: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Thu, 23 Jul 2020 12:25:17 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 22 Jul 2020 22:45:59 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Wed, Jul 22, 2020 at 10:43 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:  
> > >
> > > On Wed, Jul 22, 2020 at 6:53 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 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.
> > > >
> > > > Depending on the enabled channels, the  location of the timestamp
> > > > can be at various aligned offsets through the buffer.  As such we
> > > > any use of a structure to enforce this alignment would incorrectly
> > > > suggest a single location for the timestamp.    
> > >
> > > ...
> > >    
> > > > +       /* Ensure timestamp will be naturally aligned if present */
> > > > +       u8 buffer[24] __aligned(8);    
> > >
> > > Why can't we use proper structure here?
> > >    
> > > > @@ -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];    
> > >
> > > Seems even the old comment shows how it should look like...    
> > 
> > I think I understand now. Basically it's a dynamic amount of channels
> > (up to 6) before you get a timestamp.
> >   
> Exactly.  Comment is giving the largest it can be, not what is needed for
> a given configuration of the device.
> 
> Should indeed drop that comment.  Obviously went into automated mode and stopped
> actually reading what was in front of me.

I've adjusted the comment as requested by Andy (and moved it!).  Fits
under Andy's class 2 so applied to the togreg branch of iio.git and marked
for stable.  No great rush for this, beyond the fact that I'll keep forgetting
to actually sort these out!

Thanks,

Jonathan

> 
> Jonathan
> 
> 

Re: [PATCH v3 15/27] iio:imu:bmi160 Fix alignment and data leak issues

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:51 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to a suitable 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.
> 
> In this driver, depending on which channels are enabled, the timestamp
> can be in a number of locations.  Hence we cannot use a structure
> to specify the datalayout without it being missleading.
> 
> 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>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I applied this one then backed it out after realising that the device doesn't
have 9 channels despite the comment in place saying it does.
This particular device only has an accelerometer and a gyroscope, each
with 3 axes.

Daniel, can you confirm my interpretation on that?
I think we only need a buffer with space for
6 __le16 channels, 4 bytes padding and an s64 for the
timestamp.

So __le16 buffer[12].

Thanks,

Jonathan


> ---
>  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 a82e040bd109..d29f1b5d1658 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -10,6 +10,8 @@ struct bmi160_data {
>  	struct iio_trigger *trig;
>  	struct regulator_bulk_data supplies[2];
>  	struct iio_mount_matrix orientation;
> +	/* 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 222ebb26f013..86cfd75ea125 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -427,7 +427,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;
> @@ -438,10 +437,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;

Re: [PATCH v3 16/27] iio:imu:st_lsm6dsx Fix alignment and data leak issues

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:50:52 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> As Lars also noted this anti pattern can involve a leak of data to
> userspace and that indeed can happen here.  We close both issues by
> moving to an array of suitable structures in the iio_priv() data.
> 
> This data is allocated with kzalloc so no data can leak apart from
> previous readings.
> 
> For the tagged path the data is aligned by using __aligned(8) for
> the buffer on the stack.
> 
> There has been a lot of churn in this driver, so likely backports
> may be needed for stable.
> 
> Fixes: 290a6ce11d93 ("iio: imu: add support to lsm6dsx driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

As Lorenzo expressed he was happy with the approach in v1, I've applied
this with an additional comment added for the aligned u8 buffer to
explain what is going on there.

Applied to the togreg branch of iio.git and marked for stable.
It's late enough in the cycle that this can wait for the next merge window.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  6 ++++
>  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 36 ++++++++++---------
>  2 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index d80ba2e688ed..9275346a9cc1 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -383,6 +383,7 @@ struct st_lsm6dsx_sensor {
>   * @iio_devs: Pointers to acc/gyro iio_dev instances.
>   * @settings: Pointer to the specific sensor settings in use.
>   * @orientation: sensor chip orientation relative to main hardware.
> + * @scan: Temporary buffers used to align data before iio_push_to_buffers()
>   */
>  struct st_lsm6dsx_hw {
>  	struct device *dev;
> @@ -411,6 +412,11 @@ struct st_lsm6dsx_hw {
>  	const struct st_lsm6dsx_settings *settings;
>  
>  	struct iio_mount_matrix orientation;
> +	/* Ensure natural alignment of buffer elements */
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan[3];
>  };
>  
>  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 7de10bd636ea..9e0404e3581d 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -353,9 +353,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;
> @@ -416,19 +413,22 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>  
>  		while (acc_sip > 0 || gyro_sip > 0 || ext_sip > 0) {
>  			if (gyro_sip > 0 && !(sip % gyro_sensor->decimator)) {
> -				memcpy(gyro_buff, &hw->buff[offset],
> -				       ST_LSM6DSX_SAMPLE_SIZE);
> -				offset += ST_LSM6DSX_SAMPLE_SIZE;
> +				memcpy(hw->scan[ST_LSM6DSX_ID_GYRO].channels,
> +				       &hw->buff[offset],
> +				       sizeof(hw->scan[ST_LSM6DSX_ID_GYRO].channels));
> +				offset += sizeof(hw->scan[ST_LSM6DSX_ID_GYRO].channels);
>  			}
>  			if (acc_sip > 0 && !(sip % acc_sensor->decimator)) {
> -				memcpy(acc_buff, &hw->buff[offset],
> -				       ST_LSM6DSX_SAMPLE_SIZE);
> -				offset += ST_LSM6DSX_SAMPLE_SIZE;
> +				memcpy(hw->scan[ST_LSM6DSX_ID_ACC].channels,
> +				       &hw->buff[offset],
> +				       sizeof(hw->scan[ST_LSM6DSX_ID_ACC].channels));
> +				offset += sizeof(hw->scan[ST_LSM6DSX_ID_ACC].channels);
>  			}
>  			if (ext_sip > 0 && !(sip % ext_sensor->decimator)) {
> -				memcpy(ext_buff, &hw->buff[offset],
> -				       ST_LSM6DSX_SAMPLE_SIZE);
> -				offset += ST_LSM6DSX_SAMPLE_SIZE;
> +				memcpy(hw->scan[ST_LSM6DSX_ID_EXT0].channels,
> +				       &hw->buff[offset],
> +				       sizeof(hw->scan[ST_LSM6DSX_ID_EXT0].channels));
> +				offset += sizeof(hw->scan[ST_LSM6DSX_ID_EXT0].channels);
>  			}
>  
>  			if (ts_sip-- > 0) {
> @@ -458,19 +458,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->scan[ST_LSM6DSX_ID_GYRO],
> +					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->scan[ST_LSM6DSX_ID_ACC],
> +					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->scan[ST_LSM6DSX_ID_EXT0],
> +					ext_sensor->ts_ref + ts);
>  				ext_sip--;
>  			}
>  			sip++;
> @@ -555,7 +558,8 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
>  {
>  	u16 pattern_len = hw->sip * ST_LSM6DSX_TAGGED_SAMPLE_SIZE;
>  	u16 fifo_len, fifo_diff_mask;
> -	u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE], tag;
> +	u8 iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
> +	u8 tag;
>  	bool reset_ts = false;
>  	int i, err, read_len;
>  	__le16 fifo_status;

Re: [PATCH v3 24/27] iio:adc:ti-adc0832 Fix alignment issue with timestamp

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:51:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> 
> 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.
> 
> In this case the postioning of the timestamp is depends on what
> other channels are enabled. As such we cannot use a structure to
> make the alignment explicit as it would be missleading by suggesting
> only one possible location for the timestamp.
> 
> Fixes: 815bbc87462a ("iio: ti-adc0832: add triggered buffer support")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Expanded the comment for the data[24] element to explain that it is the
maximum length that may be used, and less may be needed if not all
channels are enabled as long as the timestamp remains naturally aligned.
(case 2 of Andy's review)

Thanks,

Jonathan

> ---
>  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 c7a085dce1f4..71d49f97ab9c 100644
> --- a/drivers/iio/adc/ti-adc0832.c
> +++ b/drivers/iio/adc/ti-adc0832.c
> @@ -29,6 +29,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];
> @@ -200,7 +202,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;
>  
> @@ -218,10 +219,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);

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

[Jonathan Cameron]

On Wed, 22 Jul 2020 16:51:01 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One of a class of bugs pointed out by Lars in a recent review.
> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> to the size of the timestamp (8 bytes).  This is not guaranteed in
> this driver which uses an array of smaller elements on the stack.
> 
> 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.
> 
> In this case the timestamp location depends on what other channels
> are enabled. As such we can't use a structure without misleading
> by suggesting only one possible timestamp location.
> 
> Fixes: 50a6edb1b6e0 ("iio: adc: add ADC12130/ADC12132/ADC12138 ADC driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied with expand comment.

Thanks,

Jonathan

> ---
>  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 e485719cd2c4..16f4fd7a04d9 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);

Re: [PATCH v3 10/27] iio:light:rpr0521 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Thu, 23 Jul 2020 12:29:39 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 22 Jul 2020 22:47:46 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Wed, Jul 22, 2020 at 6:53 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().    
> >   
> > > This data is allocated with kzalloc so no data can leak appart    
> > 
> > apart
> >   
> > > from previous readings and in this case the status byte from the device.
> > >
> > > The forced alignment of ts is not necessary in this case but it
> > > potentially makes the code less fragile.    
> > 
> > ...
> >   
> > > +        * Note that the read will put garbage data into
> > > +        * the padding but this should not be a problem    
> >   
> > > +               u8 garbage;    
> >   
> > >         err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> > > -               &buffer,
> > > +               data->scan.channels,
> > >                 (3 * 2) + 1);   /* 3 * 16-bit + (discarded) int clear reg. */    
> > 
> > But can't we read the interrupt clear register separately?
> >   
> 
> Potentially though I have no idea if there is an odd quirk there. I'm not
> immediately seeing anything on the datasheet about it.
> Would need a tested-by from someone with hardware to confirm it is fine to
> do it as two reads though.
> 
> Would be nice to get rid of the non standard handling so well worth
> pursuing in v4 of this set.

Mikko, came back to me off list on this (was stuck with an evil email client)
The cost would be significant of doing an extra read.  20 I2C clock cycles.
As such, I'd rather keep the unusual handling in here.

Will fix the typo and carry this one forwards to v4.

thanks,

Jonathan

> 
> Jonathan
> 
> 

Re: [PATCH v3 17/27] iio:imu:inv_mpu6050 Fix dma and ts alignment and data leak issues.

[Jonathan Cameron]

On Fri, 24 Jul 2020 08:27:13 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:

> Hi Jonathan,
> 
> looks perfect for me.
> 
> Thanks for the fix,

No problem.

Applied with a slightly expanded comment on the length to explain
that less may be needed if not all channels are enabled as long
as the timestamp alignment is maintained.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to poke it.

thanks,

Jonathan

> JB
> 
> Reviewed-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> 
> 
> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Cameron <jic23@kernel.org>
> Sent: Wednesday, July 22, 2020 17:50
> To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald <pmeerw@pmeerw.net>; Jonathan Cameron <Jonathan.Cameron@huawei.com>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> Subject: [PATCH v3 17/27] 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>
> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 +++++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 12 +++++-------
>  2 files changed, 10 insertions(+), 10 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 b533fa2dad0a..d8e6b88ddffc 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,11 @@ 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,
> -                                 INV_MPU6050_FIFO_COUNT_BYTE);
> +       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 +180,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 +189,7 @@ 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 v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.

[Jonathan Cameron]

On Fri, 24 Jul 2020 08:29:56 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:

> Hi Jonathan,
> 
> perfect.
> 
> Thanks for the fix,
> JB
> 
> Reviewed-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> 
> 
> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Cameron <jic23@kernel.org>
> Sent: Wednesday, July 22, 2020 17:50
> To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald <pmeerw@pmeerw.net>; Jonathan Cameron <Jonathan.Cameron@huawei.com>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> Subject: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads. 
>  
>  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>
> 
> We should not be assuming that we are reading a sequence of
> registers as here we are doing a read of a lot of data from
> a single register address.
> 
> Suggested-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Applied to the togreg branch of iio.git.  I've explicitly added that
this one isn't marked for stable as it doesn't actually have
any affect beyond being semantically correct.

thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index d8e6b88ddffc..45c37525c2f1 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -179,8 +179,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>          nb = fifo_count / bytes_per_datum;
>          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,
> -                                         st->data, bytes_per_datum);
> +               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
> +                                          st->data, bytes_per_datum);
>                  if (result)
>                          goto flush_fifo;
>                  /* skip first samples if needed */

Re: [PATCH v3 19/27] iio:pressure:mpl3115 Force alignment of buffer

[Jonathan Cameron]

On Thu, 23 Jul 2020 13:31:42 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 23 Jul 2020 15:17:16 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > 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.    
> > 
> > I guess all non-standard cases (where no struct is applicable) deserve
> > for better comment in the code.
> >   
> 
> Sure can do that. I guess that will mean people actually think about
> it whilst copying examples form old into new drivers.
> 
> In many ways the no struct applicable is the 'standard' case. It's
> mere coincidence that in some drivers there are sufficiently few
> channels that the struct 'trick' works.  All the other channels
> are moving around even in those cases, it's just the timestamp
> that happens to only have one valid location.
> 
> I'll probably do a v4 at the weekend.
Well a weekend anyway :)
This one is sufficiently unusual I will bring it back for a v4 with
additional information in the comment.
> 
> Jonathan
> 
> 
> 

Re: [PATCH v3 22/27] iio:adc:ti-adc084s021 Tidy up endian types

[Jonathan Cameron]

On Sun, 13 Sep 2020 13:06:04 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> On Sun, 13 Sep 2020 13:01:46 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Sun, 9 Aug 2020 18:47:55 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Wed, 22 Jul 2020 16:50:58 +0100
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >     
> > > > 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>      
> > > 
> > > I'm a bit in 2 minds about this one.  The exact warning will change as the
> > > result of the previous patch, but the problem is not introduced by that.
> > > Technically it's not a 'fix' so I'll hold this one for now.    
> > 
> > Applied this one to the togreg branch of iio.git and pushed out as testing for
> > the autobuilders to play with it.  
> And failed my local tests. Precursor not in my branch yet.
> oops.
> Dropped again for now.
And applied again successfully.

Jonathan

> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > 
> > > Jonathan
> > >     
> > > > ---
> > > >  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 dfba34834a57..fb14b92fa6e7 100644
> > > > --- a/drivers/iio/adc/ti-adc084s021.c
> > > > +++ b/drivers/iio/adc/ti-adc084s021.c
> > > > @@ -70,11 +70,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);
> > > > @@ -82,7 +81,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;
> > > >  }
> > > > @@ -93,6 +92,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:
> > > > @@ -107,13 +107,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;      
> > >     
> >   
>