[PATCH 00/11] IIO: 1st set of timestamp alignment fixes.

[PATCH 00/11] IIO: 1st set of timestamp alignment fixes.

[jic23]

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

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

Jonathan Cameron (11):
  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:health:afe4403 Fix timestamp alignment and prevent data leak.
  iio:health:afe4404 Fix timestamp alignment and prevent data leak.
  iio:chemical:ccs811: Fix timestamp alignment and prevent data leak.
  iio:chemical:sps30: Fix timestamp alignment
  iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.

 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/chemical/ccs811.c         | 13 +++++++++----
 drivers/iio/chemical/pms7003.c        | 17 ++++++++++++-----
 drivers/iio/chemical/sps30.c          |  9 ++++++---
 drivers/iio/gyro/itg3200_buffer.c     | 15 +++++++++++----
 drivers/iio/health/afe4403.c          |  9 ++++++---
 drivers/iio/health/afe4404.c          |  8 +++++---
 drivers/iio/proximity/mb1232.c        | 17 +++++++++--------
 11 files changed, 101 insertions(+), 45 deletions(-)

-- 
2.26.2

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

[jic23]

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.

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 0b876b2dc5bd..f989e6ffda88 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;
+	} 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.26.2

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

[jic23]

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.

Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/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 00e100fc845a..704867ffda7a 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;
+	} 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.26.2

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

[jic23]

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.

Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware fifo")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: 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 121b4e89f038..b92e5a22feef 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;
+	} 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],
+			       2);
 
-		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.26.2

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

[jic23]

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.

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 8b5a6aff9bf4..b48caece170e 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;
+	} 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.26.2

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

[jic23]

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.

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..cd405801e61e 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;
+	} 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.26.2

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

[jic23]

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.

Fixes: 16b05261537e ("mb1232.c: add distance iio sensor with i2c")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andreas Klinger <ak@it-klinger.de>
---
 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 166b3e6d7db8..5ec2d306f1c1 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;
+	} 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.26.2

[PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[jic23]

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

Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403 heart monitor")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andrew F. Davis <afd@ti.com>
---
 drivers/iio/health/afe4403.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
index e9f87e42ff4f..a3507624b30f 100644
--- a/drivers/iio/health/afe4403.c
+++ b/drivers/iio/health/afe4403.c
@@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[] = {
  * @regulator: Pointer to the regulator for the IC
  * @trig: IIO trigger for this device
  * @irq: ADC_RDY line interrupt number
+ * @buffer: Used to construct data layout to push into IIO buffer.
  */
 struct afe4403_data {
 	struct device *dev;
@@ -74,6 +75,8 @@ struct afe4403_data {
 	struct regulator *regulator;
 	struct iio_trigger *trig;
 	int irq;
+	/* Ensure suitable alignment for timestamp */
+	s32 buffer[8] __aligned(8);
 };
 
 enum afe4403_chan_id {
@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct afe4403_data *afe = iio_priv(indio_dev);
 	int ret, bit, i = 0;
-	s32 buffer[8];
 	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
 	u8 rx[3];
 
@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)
 		if (ret)
 			goto err;
 
-		buffer[i++] = get_unaligned_be24(&rx[0]);
+		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
 	}
 
 	/* Disable reading from the device */
@@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)
 	if (ret)
 		goto err;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
+					   pf->timestamp);
 err:
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.26.2

[PATCH 08/11] iio:health:afe4404 Fix timestamp alignment and prevent data leak.

[jic23]

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

Fixes: 87aec56e27ef ("iio: health: Add driver for the TI AFE4404 heart monitor")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andrew F. Davis <afd@ti.com>
---
 drivers/iio/health/afe4404.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/health/afe4404.c b/drivers/iio/health/afe4404.c
index e728bbb21ca8..cebb1fd4d0b1 100644
--- a/drivers/iio/health/afe4404.c
+++ b/drivers/iio/health/afe4404.c
@@ -83,6 +83,7 @@ static const struct reg_field afe4404_reg_fields[] = {
  * @regulator: Pointer to the regulator for the IC
  * @trig: IIO trigger for this device
  * @irq: ADC_RDY line interrupt number
+ * @buffer: Used to construct a scan to push to the iio buffer.
  */
 struct afe4404_data {
 	struct device *dev;
@@ -91,6 +92,7 @@ struct afe4404_data {
 	struct regulator *regulator;
 	struct iio_trigger *trig;
 	int irq;
+	s32 buffer[10] __aligned(8);
 };
 
 enum afe4404_chan_id {
@@ -328,17 +330,17 @@ static irqreturn_t afe4404_trigger_handler(int irq, void *private)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct afe4404_data *afe = iio_priv(indio_dev);
 	int ret, bit, i = 0;
-	s32 buffer[10];
 
 	for_each_set_bit(bit, indio_dev->active_scan_mask,
 			 indio_dev->masklength) {
 		ret = regmap_read(afe->regmap, afe4404_channel_values[bit],
-				  &buffer[i++]);
+				  &afe->buffer[i++]);
 		if (ret)
 			goto err;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
+					   pf->timestamp);
 err:
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.26.2

[PATCH 09/11] iio:chemical:ccs811: Fix timestamp alignment and prevent data leak.

[jic23]

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.

Fixes: 283d26917ad6 ("iio: chemical: ccs811: Add triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.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 3ecd633f9ed3..fe1901be320d 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;
+	} 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.26.2

[PATCH 10/11] iio:chemical:sps30: Fix timestamp alignment

[jic23]

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.

Fixes: 232e0f6ddeae ("iio: chemical: add support for Sensirion SPS30 sensor")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Tomasz Duszynski <tduszyns@gmail.com>
---
 drivers/iio/chemical/sps30.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
index acb9f8ecbb3d..a88c1fb875a0 100644
--- a/drivers/iio/chemical/sps30.c
+++ b/drivers/iio/chemical/sps30.c
@@ -230,15 +230,18 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct sps30_state *state = iio_priv(indio_dev);
 	int ret;
-	s32 data[4 + 2]; /* PM1, PM2P5, PM4, PM10, timestamp */
+	struct {
+		s32 data[4]; /* PM1, PM2P5, PM4, PM10 */
+		s64 ts;
+	} scan;
 
 	mutex_lock(&state->lock);
-	ret = sps30_do_meas(state, data, 4);
+	ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
 	mutex_unlock(&state->lock);
 	if (ret)
 		goto err;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data,
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
 					   iio_get_time_ns(indio_dev));
 err:
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.26.2

[PATCH 11/11] iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.

[jic23]

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.

Fixes: a1d642266c14 ("iio: chemical: add support for Plantower PMS7003 sensor")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Tomasz Duszynski <tduszyns@gmail.com>
---
 drivers/iio/chemical/pms7003.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/chemical/pms7003.c b/drivers/iio/chemical/pms7003.c
index 23c9ab252470..07bb90d72434 100644
--- a/drivers/iio/chemical/pms7003.c
+++ b/drivers/iio/chemical/pms7003.c
@@ -73,6 +73,11 @@ struct pms7003_state {
 	struct pms7003_frame frame;
 	struct completion frame_ready;
 	struct mutex lock; /* must be held whenever state gets touched */
+	/* Used to construct scan to push to the IIO buffer */
+	struct {
+		u16 data[3]; /* PM1, PM2P5, PM10 */
+		s64 ts;
+	} scan;
 };
 
 static int pms7003_do_cmd(struct pms7003_state *state, enum pms7003_cmd cmd)
@@ -104,7 +109,6 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct pms7003_state *state = iio_priv(indio_dev);
 	struct pms7003_frame *frame = &state->frame;
-	u16 data[3 + 1 + 4]; /* PM1, PM2P5, PM10, padding, timestamp */
 	int ret;
 
 	mutex_lock(&state->lock);
@@ -114,12 +118,15 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)
 		goto err;
 	}
 
-	data[PM1] = pms7003_get_pm(frame->data + PMS7003_PM1_OFFSET);
-	data[PM2P5] = pms7003_get_pm(frame->data + PMS7003_PM2P5_OFFSET);
-	data[PM10] = pms7003_get_pm(frame->data + PMS7003_PM10_OFFSET);
+	state->scan.data[PM1] =
+		pms7003_get_pm(frame->data + PMS7003_PM1_OFFSET);
+	state->scan.data[PM2P5] =
+		pms7003_get_pm(frame->data + PMS7003_PM2P5_OFFSET);
+	state->scan.data[PM10] =
+		pms7003_get_pm(frame->data + PMS7003_PM10_OFFSET);
 	mutex_unlock(&state->lock);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data,
+	iio_push_to_buffers_with_timestamp(indio_dev, &state->scan,
 					   iio_get_time_ns(indio_dev));
 err:
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.26.2

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

[Lars-Peter Clausen]

On 5/17/20 7:29 PM, 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.
>
> Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> ---
>   drivers/iio/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 00e100fc845a..704867ffda7a 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;
> +	} buffer;


I feel we should have a macro for this.

DECLARE_IIO_BUFFER_WITH_TIMESTAMP(buffer, __be16, 3);

The name is maybe a bit too long.

And potentially also DECLARE_IIO_BUFFER_WITH_TIMESTAMP_ON_STACK() which 
initializes it to zero.

>   };
>   
>    /**
> @@ -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 11/11] iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.

[Tomasz Duszynski]

On Sun, May 17, 2020 at 06:30:00PM +0100, 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.
>
> Fixes: a1d642266c14 ("iio: chemical: add support for Plantower PMS7003 sensor")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Tomasz Duszynski <tduszyns@gmail.com>
> ---
>  drivers/iio/chemical/pms7003.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/chemical/pms7003.c b/drivers/iio/chemical/pms7003.c
> index 23c9ab252470..07bb90d72434 100644
> --- a/drivers/iio/chemical/pms7003.c
> +++ b/drivers/iio/chemical/pms7003.c
> @@ -73,6 +73,11 @@ struct pms7003_state {
>  	struct pms7003_frame frame;
>  	struct completion frame_ready;
>  	struct mutex lock; /* must be held whenever state gets touched */
> +	/* Used to construct scan to push to the IIO buffer */
> +	struct {
> +		u16 data[3]; /* PM1, PM2P5, PM10 */
> +		s64 ts;
> +	} scan;
>  };
>
>  static int pms7003_do_cmd(struct pms7003_state *state, enum pms7003_cmd cmd)
> @@ -104,7 +109,6 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct pms7003_state *state = iio_priv(indio_dev);
>  	struct pms7003_frame *frame = &state->frame;
> -	u16 data[3 + 1 + 4]; /* PM1, PM2P5, PM10, padding, timestamp */
>  	int ret;
>
>  	mutex_lock(&state->lock);
> @@ -114,12 +118,15 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)
>  		goto err;
>  	}
>
> -	data[PM1] = pms7003_get_pm(frame->data + PMS7003_PM1_OFFSET);
> -	data[PM2P5] = pms7003_get_pm(frame->data + PMS7003_PM2P5_OFFSET);
> -	data[PM10] = pms7003_get_pm(frame->data + PMS7003_PM10_OFFSET);
> +	state->scan.data[PM1] =
> +		pms7003_get_pm(frame->data + PMS7003_PM1_OFFSET);
> +	state->scan.data[PM2P5] =
> +		pms7003_get_pm(frame->data + PMS7003_PM2P5_OFFSET);
> +	state->scan.data[PM10] =
> +		pms7003_get_pm(frame->data + PMS7003_PM10_OFFSET);
>  	mutex_unlock(&state->lock);
>
> -	iio_push_to_buffers_with_timestamp(indio_dev, data,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &state->scan,
>  					   iio_get_time_ns(indio_dev));
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);
> --
> 2.26.2
>

Thanks for the fix.
Acked-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>

Re: [PATCH 10/11] iio:chemical:sps30: Fix timestamp alignment

[Tomasz Duszynski]

On Sun, May 17, 2020 at 06:29:59PM +0100, 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.
>
> Fixes: 232e0f6ddeae ("iio: chemical: add support for Sensirion SPS30 sensor")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Tomasz Duszynski <tduszyns@gmail.com>
> ---
>  drivers/iio/chemical/sps30.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> index acb9f8ecbb3d..a88c1fb875a0 100644
> --- a/drivers/iio/chemical/sps30.c
> +++ b/drivers/iio/chemical/sps30.c
> @@ -230,15 +230,18 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct sps30_state *state = iio_priv(indio_dev);
>  	int ret;
> -	s32 data[4 + 2]; /* PM1, PM2P5, PM4, PM10, timestamp */
> +	struct {
> +		s32 data[4]; /* PM1, PM2P5, PM4, PM10 */
> +		s64 ts;
> +	} scan;
>
>  	mutex_lock(&state->lock);
> -	ret = sps30_do_meas(state, data, 4);
> +	ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
>  	mutex_unlock(&state->lock);
>  	if (ret)
>  		goto err;
>
> -	iio_push_to_buffers_with_timestamp(indio_dev, data,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
>  					   iio_get_time_ns(indio_dev));
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);
> --
> 2.26.2
>

Thanks for the fix.
Acked-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>

Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Andrew F. Davis]

On 5/17/20 1:29 PM, 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 appart from previous readings.
> 
> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403 heart monitor")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andrew F. Davis <afd@ti.com>
> ---
>  drivers/iio/health/afe4403.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
> index e9f87e42ff4f..a3507624b30f 100644
> --- a/drivers/iio/health/afe4403.c
> +++ b/drivers/iio/health/afe4403.c
> @@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[] = {
>   * @regulator: Pointer to the regulator for the IC
>   * @trig: IIO trigger for this device
>   * @irq: ADC_RDY line interrupt number
> + * @buffer: Used to construct data layout to push into IIO buffer.
>   */
>  struct afe4403_data {
>  	struct device *dev;
> @@ -74,6 +75,8 @@ struct afe4403_data {
>  	struct regulator *regulator;
>  	struct iio_trigger *trig;
>  	int irq;
> +	/* Ensure suitable alignment for timestamp */
> +	s32 buffer[8] __aligned(8);


One of those fancy structs with the timestamp specified would be nice
here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.

Other than that everything looks good.

Andrew


>  };
>  
>  enum afe4403_chan_id {
> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct afe4403_data *afe = iio_priv(indio_dev);
>  	int ret, bit, i = 0;
> -	s32 buffer[8];
>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
>  	u8 rx[3];
>  
> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)
>  		if (ret)
>  			goto err;
>  
> -		buffer[i++] = get_unaligned_be24(&rx[0]);
> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
>  	}
>  
>  	/* Disable reading from the device */
> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)
>  	if (ret)
>  		goto err;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
> +					   pf->timestamp);
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);
>  
> 

Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>On 5/17/20 1:29 PM, 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 appart from previous readings.
>> 
>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403
>heart monitor")
>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Cc: Andrew F. Davis <afd@ti.com>
>> ---
>>  drivers/iio/health/afe4403.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/iio/health/afe4403.c
>b/drivers/iio/health/afe4403.c
>> index e9f87e42ff4f..a3507624b30f 100644
>> --- a/drivers/iio/health/afe4403.c
>> +++ b/drivers/iio/health/afe4403.c
>> @@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[]
>= {
>>   * @regulator: Pointer to the regulator for the IC
>>   * @trig: IIO trigger for this device
>>   * @irq: ADC_RDY line interrupt number
>> + * @buffer: Used to construct data layout to push into IIO buffer.
>>   */
>>  struct afe4403_data {
>>  	struct device *dev;
>> @@ -74,6 +75,8 @@ struct afe4403_data {
>>  	struct regulator *regulator;
>>  	struct iio_trigger *trig;
>>  	int irq;
>> +	/* Ensure suitable alignment for timestamp */
>> +	s32 buffer[8] __aligned(8);
>
>
>One of those fancy structs with the timestamp specified would be nice
>here like the other patches. IIRC we have 6 s32 channels, plus a s64
>ts.

I think we may only have some of those channels enabled.  So ts may be it several
locations in the buffer. 

Hence we could use the structure approach but it might give a false sense
of what is going on. 

J
>
>Other than that everything looks good.
>
>Andrew
>
>
>>  };
>>  
>>  enum afe4403_chan_id {
>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int
>irq, void *private)
>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>  	struct afe4403_data *afe = iio_priv(indio_dev);
>>  	int ret, bit, i = 0;
>> -	s32 buffer[8];
>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
>>  	u8 rx[3];
>>  
>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int
>irq, void *private)
>>  		if (ret)
>>  			goto err;
>>  
>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
>>  	}
>>  
>>  	/* Disable reading from the device */
>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int
>irq, void *private)
>>  	if (ret)
>>  		goto err;
>>  
>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>pf->timestamp);
>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
>> +					   pf->timestamp);
>>  err:
>>  	iio_trigger_notify_done(indio_dev->trig);
>>  
>> 

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

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

[Jonathan Cameron]

On Sun, 17 May 2020 20:57:26 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 5/17/20 7:29 PM, 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.
> >
> > Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Peter Meerwald <pmeerw@pmeerw.net>
> > ---
> >   drivers/iio/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 00e100fc845a..704867ffda7a 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;
> > +	} buffer;  
> 
> 
> I feel we should have a macro for this.
> 
> DECLARE_IIO_BUFFER_WITH_TIMESTAMP(buffer, __be16, 3);
> 
> The name is maybe a bit too long.

It runs into the issue we had with the afe4403 and other devices
that are happy to do very different numbers of enabled channels.
Such a macro would imply that the timestamp will always be written
at a fixed location, which isn't true.  So far, I've made sure
we didn't do anything like this unless the number of channels was
small enough there was only one possible location for the timestamp
(now we insist that at least one channel is enabled to start a buffer).

Maybe we just deal with that with some suitable documentation though.

> 
> And potentially also DECLARE_IIO_BUFFER_WITH_TIMESTAMP_ON_STACK() which 
> initializes it to zero.

The thing there is we should only need to initialize it to zero if there
are holes.  If we always write data up to the 8 byte boundary
it won't matter if the timestamp is disabled or not, I don't think we
will get a data leak.  We can't even do some magic in the call to
identify if there are potential holes because we can't tell if all
the channels will be written or not...

I'm not totally convinced hiding what is going on behind a macro
is a good idea.  Sometimes bashing people over the head with the
fact there are some non obvious requirements is a good idea.

Not sure...

J


> 
> >   };
> >   
> >    /**
> > @@ -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 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Andrew F. Davis]

On 5/18/20 12:06 PM, Jonathan Cameron wrote:
> 
> 
> On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>> On 5/17/20 1:29 PM, 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 appart from previous readings.
>>>
>>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403
>> heart monitor")
>>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Cc: Andrew F. Davis <afd@ti.com>
>>> ---
>>>  drivers/iio/health/afe4403.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/health/afe4403.c
>> b/drivers/iio/health/afe4403.c
>>> index e9f87e42ff4f..a3507624b30f 100644
>>> --- a/drivers/iio/health/afe4403.c
>>> +++ b/drivers/iio/health/afe4403.c
>>> @@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[]
>> = {
>>>   * @regulator: Pointer to the regulator for the IC
>>>   * @trig: IIO trigger for this device
>>>   * @irq: ADC_RDY line interrupt number
>>> + * @buffer: Used to construct data layout to push into IIO buffer.
>>>   */
>>>  struct afe4403_data {
>>>  	struct device *dev;
>>> @@ -74,6 +75,8 @@ struct afe4403_data {
>>>  	struct regulator *regulator;
>>>  	struct iio_trigger *trig;
>>>  	int irq;
>>> +	/* Ensure suitable alignment for timestamp */
>>> +	s32 buffer[8] __aligned(8);
>>
>>
>> One of those fancy structs with the timestamp specified would be nice
>> here like the other patches. IIRC we have 6 s32 channels, plus a s64
>> ts.
> 
> I think we may only have some of those channels enabled.  So ts may be it several
> locations in the buffer. 
> 

Might have been better to have the ts at the beginning, could have also
helped with alignment for when an odd number of channels are enabled.

> Hence we could use the structure approach but it might give a false sense
> of what is going on. 
> 

That's true, it can always be cleaned later then.

Andrew

> J
>>
>> Other than that everything looks good.
>>
>> Andrew
>>
>>
>>>  };
>>>  
>>>  enum afe4403_chan_id {
>>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int
>> irq, void *private)
>>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>>  	struct afe4403_data *afe = iio_priv(indio_dev);
>>>  	int ret, bit, i = 0;
>>> -	s32 buffer[8];
>>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
>>>  	u8 rx[3];
>>>  
>>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int
>> irq, void *private)
>>>  		if (ret)
>>>  			goto err;
>>>  
>>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
>>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
>>>  	}
>>>  
>>>  	/* Disable reading from the device */
>>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int
>> irq, void *private)
>>>  	if (ret)
>>>  		goto err;
>>>  
>>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>> pf->timestamp);
>>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
>>> +					   pf->timestamp);
>>>  err:
>>>  	iio_trigger_notify_done(indio_dev->trig);
>>>  
>>>
> 

Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>
>
>On 5/18/20 12:06 PM, Jonathan Cameron wrote:
>> 
>> 
>> On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>>> On 5/17/20 1:29 PM, 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 appart from previous readings.
>>>>
>>>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403
>>> heart monitor")
>>>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> Cc: Andrew F. Davis <afd@ti.com>
>>>> ---
>>>>  drivers/iio/health/afe4403.c | 9 ++++++---
>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/health/afe4403.c
>>> b/drivers/iio/health/afe4403.c
>>>> index e9f87e42ff4f..a3507624b30f 100644
>>>> --- a/drivers/iio/health/afe4403.c
>>>> +++ b/drivers/iio/health/afe4403.c
>>>> @@ -65,6 +65,7 @@ static const struct reg_field
>afe4403_reg_fields[]
>>> = {
>>>>   * @regulator: Pointer to the regulator for the IC
>>>>   * @trig: IIO trigger for this device
>>>>   * @irq: ADC_RDY line interrupt number
>>>> + * @buffer: Used to construct data layout to push into IIO buffer.
>>>>   */
>>>>  struct afe4403_data {
>>>>  	struct device *dev;
>>>> @@ -74,6 +75,8 @@ struct afe4403_data {
>>>>  	struct regulator *regulator;
>>>>  	struct iio_trigger *trig;
>>>>  	int irq;
>>>> +	/* Ensure suitable alignment for timestamp */
>>>> +	s32 buffer[8] __aligned(8);
>>>
>>>
>>> One of those fancy structs with the timestamp specified would be
>nice
>>> here like the other patches. IIRC we have 6 s32 channels, plus a s64
>>> ts.
>> 
>> I think we may only have some of those channels enabled.  So ts may
>be it several
>> locations in the buffer. 
>> 
>
>Might have been better to have the ts at the beginning, could have also
>helped with alignment for when an odd number of channels are enabled.

Perhaps but for many use cases we don't turn timestamps on and we would need to pad
 anyway for alignment of the fifo. 

>
>> Hence we could use the structure approach but it might give a false
>sense
>> of what is going on. 
>> 
>
>That's true, it can always be cleaned later then.

Agreed. This is definitely something we can revisit sometime in the future. 

J
>
>Andrew
>
>> J
>>>
>>> Other than that everything looks good.
>>>
>>> Andrew
>>>
>>>
>>>>  };
>>>>  
>>>>  enum afe4403_chan_id {
>>>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int
>>> irq, void *private)
>>>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>>>  	struct afe4403_data *afe = iio_priv(indio_dev);
>>>>  	int ret, bit, i = 0;
>>>> -	s32 buffer[8];
>>>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
>>>>  	u8 rx[3];
>>>>  
>>>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int
>>> irq, void *private)
>>>>  		if (ret)
>>>>  			goto err;
>>>>  
>>>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
>>>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
>>>>  	}
>>>>  
>>>>  	/* Disable reading from the device */
>>>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int
>>> irq, void *private)
>>>>  	if (ret)
>>>>  		goto err;
>>>>  
>>>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>>> pf->timestamp);
>>>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
>>>> +					   pf->timestamp);
>>>>  err:
>>>>  	iio_trigger_notify_done(indio_dev->trig);
>>>>  
>>>>
>> 

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

Re: [PATCH 10/11] iio:chemical:sps30: Fix timestamp alignment

[Jonathan Cameron]

On Sun, 17 May 2020 21:07:28 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> On Sun, May 17, 2020 at 06:29:59PM +0100, 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.
> >
> > Fixes: 232e0f6ddeae ("iio: chemical: add support for Sensirion SPS30 sensor")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Tomasz Duszynski <tduszyns@gmail.com>
> > ---
> >  drivers/iio/chemical/sps30.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > index acb9f8ecbb3d..a88c1fb875a0 100644
> > --- a/drivers/iio/chemical/sps30.c
> > +++ b/drivers/iio/chemical/sps30.c
> > @@ -230,15 +230,18 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
> >  	struct iio_dev *indio_dev = pf->indio_dev;
> >  	struct sps30_state *state = iio_priv(indio_dev);
> >  	int ret;
> > -	s32 data[4 + 2]; /* PM1, PM2P5, PM4, PM10, timestamp */
> > +	struct {
> > +		s32 data[4]; /* PM1, PM2P5, PM4, PM10 */
> > +		s64 ts;
> > +	} scan;
> >
> >  	mutex_lock(&state->lock);
> > -	ret = sps30_do_meas(state, data, 4);
> > +	ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
> >  	mutex_unlock(&state->lock);
> >  	if (ret)
> >  		goto err;
> >
> > -	iio_push_to_buffers_with_timestamp(indio_dev, data,
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> >  					   iio_get_time_ns(indio_dev));
> >  err:
> >  	iio_trigger_notify_done(indio_dev->trig);
> > --
> > 2.26.2
> >  
> 
> Thanks for the fix.
> Acked-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
Applied to the fixes-togreg branch of iio.git.

I'm going to pick these up as and when I get reviews for them rather than
waiting for the whole series to have suitable reviews.

They are a small part of all the instances we have of this issue
so no point in waiting.

Thanks,

Jonathan

Re: [PATCH 11/11] iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Sun, 17 May 2020 21:06:55 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> On Sun, May 17, 2020 at 06:30:00PM +0100, 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.
> >
> > Fixes: a1d642266c14 ("iio: chemical: add support for Plantower PMS7003 sensor")
> > Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Tomasz Duszynski <tduszyns@gmail.com>
> > ---
> >  drivers/iio/chemical/pms7003.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/pms7003.c b/drivers/iio/chemical/pms7003.c
> > index 23c9ab252470..07bb90d72434 100644
> > --- a/drivers/iio/chemical/pms7003.c
> > +++ b/drivers/iio/chemical/pms7003.c
> > @@ -73,6 +73,11 @@ struct pms7003_state {
> >  	struct pms7003_frame frame;
> >  	struct completion frame_ready;
> >  	struct mutex lock; /* must be held whenever state gets touched */
> > +	/* Used to construct scan to push to the IIO buffer */
> > +	struct {
> > +		u16 data[3]; /* PM1, PM2P5, PM10 */
> > +		s64 ts;
> > +	} scan;
> >  };
> >
> >  static int pms7003_do_cmd(struct pms7003_state *state, enum pms7003_cmd cmd)
> > @@ -104,7 +109,6 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)
> >  	struct iio_dev *indio_dev = pf->indio_dev;
> >  	struct pms7003_state *state = iio_priv(indio_dev);
> >  	struct pms7003_frame *frame = &state->frame;
> > -	u16 data[3 + 1 + 4]; /* PM1, PM2P5, PM10, padding, timestamp */
> >  	int ret;
> >
> >  	mutex_lock(&state->lock);
> > @@ -114,12 +118,15 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)
> >  		goto err;
> >  	}
> >
> > -	data[PM1] = pms7003_get_pm(frame->data + PMS7003_PM1_OFFSET);
> > -	data[PM2P5] = pms7003_get_pm(frame->data + PMS7003_PM2P5_OFFSET);
> > -	data[PM10] = pms7003_get_pm(frame->data + PMS7003_PM10_OFFSET);
> > +	state->scan.data[PM1] =
> > +		pms7003_get_pm(frame->data + PMS7003_PM1_OFFSET);
> > +	state->scan.data[PM2P5] =
> > +		pms7003_get_pm(frame->data + PMS7003_PM2P5_OFFSET);
> > +	state->scan.data[PM10] =
> > +		pms7003_get_pm(frame->data + PMS7003_PM10_OFFSET);
> >  	mutex_unlock(&state->lock);
> >
> > -	iio_push_to_buffers_with_timestamp(indio_dev, data,
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &state->scan,
> >  					   iio_get_time_ns(indio_dev));
> >  err:
> >  	iio_trigger_notify_done(indio_dev->trig);
> > --
> > 2.26.2
> >  
> 
> Thanks for the fix.
> Acked-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> 
Applied. Thanks,

Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Thu, 21 May 2020 15:44:43 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote:
> >
> >
> >On 5/18/20 12:06 PM, Jonathan Cameron wrote:  
> >> 
> >> 
> >> On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:  
> >>> On 5/17/20 1:29 PM, 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 appart from previous readings.
> >>>>
> >>>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403  
> >>> heart monitor")  
> >>>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> >>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>> Cc: Andrew F. Davis <afd@ti.com>
> >>>> ---
> >>>>  drivers/iio/health/afe4403.c | 9 ++++++---
> >>>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/health/afe4403.c  
> >>> b/drivers/iio/health/afe4403.c  
> >>>> index e9f87e42ff4f..a3507624b30f 100644
> >>>> --- a/drivers/iio/health/afe4403.c
> >>>> +++ b/drivers/iio/health/afe4403.c
> >>>> @@ -65,6 +65,7 @@ static const struct reg_field  
> >afe4403_reg_fields[]  
> >>> = {  
> >>>>   * @regulator: Pointer to the regulator for the IC
> >>>>   * @trig: IIO trigger for this device
> >>>>   * @irq: ADC_RDY line interrupt number
> >>>> + * @buffer: Used to construct data layout to push into IIO buffer.
> >>>>   */
> >>>>  struct afe4403_data {
> >>>>  	struct device *dev;
> >>>> @@ -74,6 +75,8 @@ struct afe4403_data {
> >>>>  	struct regulator *regulator;
> >>>>  	struct iio_trigger *trig;
> >>>>  	int irq;
> >>>> +	/* Ensure suitable alignment for timestamp */
> >>>> +	s32 buffer[8] __aligned(8);  
> >>>
> >>>
> >>> One of those fancy structs with the timestamp specified would be  
> >nice  
> >>> here like the other patches. IIRC we have 6 s32 channels, plus a s64
> >>> ts.  
> >> 
> >> I think we may only have some of those channels enabled.  So ts may  
> >be it several  
> >> locations in the buffer. 
> >>   
> >
> >Might have been better to have the ts at the beginning, could have also
> >helped with alignment for when an odd number of channels are enabled.  
> 
> Perhaps but for many use cases we don't turn timestamps on and we would need to pad
>  anyway for alignment of the fifo. 
> 
> >  
> >> Hence we could use the structure approach but it might give a false  
> >sense  
> >> of what is going on. 
> >>   
> >
> >That's true, it can always be cleaned later then.  
> 
> Agreed. This is definitely something we can revisit sometime in the future. 

Formal Ack on applying the two afe patches?

Thanks,

Jonathan
> 
> J
> >
> >Andrew
> >  
> >> J  
> >>>
> >>> Other than that everything looks good.
> >>>
> >>> Andrew
> >>>
> >>>  
> >>>>  };
> >>>>  
> >>>>  enum afe4403_chan_id {
> >>>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int  
> >>> irq, void *private)  
> >>>>  	struct iio_dev *indio_dev = pf->indio_dev;
> >>>>  	struct afe4403_data *afe = iio_priv(indio_dev);
> >>>>  	int ret, bit, i = 0;
> >>>> -	s32 buffer[8];
> >>>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
> >>>>  	u8 rx[3];
> >>>>  
> >>>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int  
> >>> irq, void *private)  
> >>>>  		if (ret)
> >>>>  			goto err;
> >>>>  
> >>>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
> >>>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
> >>>>  	}
> >>>>  
> >>>>  	/* Disable reading from the device */
> >>>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int  
> >>> irq, void *private)  
> >>>>  	if (ret)
> >>>>  		goto err;
> >>>>  
> >>>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,  
> >>> pf->timestamp);  
> >>>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
> >>>> +					   pf->timestamp);
> >>>>  err:
> >>>>  	iio_trigger_notify_done(indio_dev->trig);
> >>>>  
> >>>>  
> >>   
> 

Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Andrew F. Davis]

On 5/21/20 2:45 PM, Jonathan Cameron wrote:
> On Thu, 21 May 2020 15:44:43 +0100
> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> 
>> On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>>>
>>>
>>> On 5/18/20 12:06 PM, Jonathan Cameron wrote:  
>>>>
>>>>
>>>> On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:  
>>>>> On 5/17/20 1:29 PM, 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 appart from previous readings.
>>>>>>
>>>>>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403  
>>>>> heart monitor")  
>>>>>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>> Cc: Andrew F. Davis <afd@ti.com>
>>>>>> ---
>>>>>>  drivers/iio/health/afe4403.c | 9 ++++++---
>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/health/afe4403.c  
>>>>> b/drivers/iio/health/afe4403.c  
>>>>>> index e9f87e42ff4f..a3507624b30f 100644
>>>>>> --- a/drivers/iio/health/afe4403.c
>>>>>> +++ b/drivers/iio/health/afe4403.c
>>>>>> @@ -65,6 +65,7 @@ static const struct reg_field  
>>> afe4403_reg_fields[]  
>>>>> = {  
>>>>>>   * @regulator: Pointer to the regulator for the IC
>>>>>>   * @trig: IIO trigger for this device
>>>>>>   * @irq: ADC_RDY line interrupt number
>>>>>> + * @buffer: Used to construct data layout to push into IIO buffer.
>>>>>>   */
>>>>>>  struct afe4403_data {
>>>>>>  	struct device *dev;
>>>>>> @@ -74,6 +75,8 @@ struct afe4403_data {
>>>>>>  	struct regulator *regulator;
>>>>>>  	struct iio_trigger *trig;
>>>>>>  	int irq;
>>>>>> +	/* Ensure suitable alignment for timestamp */
>>>>>> +	s32 buffer[8] __aligned(8);  
>>>>>
>>>>>
>>>>> One of those fancy structs with the timestamp specified would be  
>>> nice  
>>>>> here like the other patches. IIRC we have 6 s32 channels, plus a s64
>>>>> ts.  
>>>>
>>>> I think we may only have some of those channels enabled.  So ts may  
>>> be it several  
>>>> locations in the buffer. 
>>>>   
>>>
>>> Might have been better to have the ts at the beginning, could have also
>>> helped with alignment for when an odd number of channels are enabled.  
>>
>> Perhaps but for many use cases we don't turn timestamps on and we would need to pad
>>  anyway for alignment of the fifo. 
>>
>>>  
>>>> Hence we could use the structure approach but it might give a false  
>>> sense  
>>>> of what is going on. 
>>>>   
>>>
>>> That's true, it can always be cleaned later then.  
>>
>> Agreed. This is definitely something we can revisit sometime in the future. 
> 
> Formal Ack on applying the two afe patches?
> 

Acked-by: Andrew F. Davis <afd@ti.com>

> Thanks,
> 
> Jonathan
>>
>> J
>>>
>>> Andrew
>>>  
>>>> J  
>>>>>
>>>>> Other than that everything looks good.
>>>>>
>>>>> Andrew
>>>>>
>>>>>  
>>>>>>  };
>>>>>>  
>>>>>>  enum afe4403_chan_id {
>>>>>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int  
>>>>> irq, void *private)  
>>>>>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>>>>>  	struct afe4403_data *afe = iio_priv(indio_dev);
>>>>>>  	int ret, bit, i = 0;
>>>>>> -	s32 buffer[8];
>>>>>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
>>>>>>  	u8 rx[3];
>>>>>>  
>>>>>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int  
>>>>> irq, void *private)  
>>>>>>  		if (ret)
>>>>>>  			goto err;
>>>>>>  
>>>>>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
>>>>>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
>>>>>>  	}
>>>>>>  
>>>>>>  	/* Disable reading from the device */
>>>>>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int  
>>>>> irq, void *private)  
>>>>>>  	if (ret)
>>>>>>  		goto err;
>>>>>>  
>>>>>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,  
>>>>> pf->timestamp);  
>>>>>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
>>>>>> +					   pf->timestamp);
>>>>>>  err:
>>>>>>  	iio_trigger_notify_done(indio_dev->trig);
>>>>>>  
>>>>>>  
>>>>   
>>
> 

Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Thu, 21 May 2020 15:05:37 -0400
"Andrew F. Davis" <afd@ti.com> wrote:

> On 5/21/20 2:45 PM, Jonathan Cameron wrote:
> > On Thu, 21 May 2020 15:44:43 +0100
> > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >   
> >> On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote:  
> >>>
> >>>
> >>> On 5/18/20 12:06 PM, Jonathan Cameron wrote:    
> >>>>
> >>>>
> >>>> On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:    
> >>>>> On 5/17/20 1:29 PM, 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 appart from previous readings.
> >>>>>>
> >>>>>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403    
> >>>>> heart monitor")    
> >>>>>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> >>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>>>> Cc: Andrew F. Davis <afd@ti.com>
> >>>>>> ---
> >>>>>>  drivers/iio/health/afe4403.c | 9 ++++++---
> >>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/iio/health/afe4403.c    
> >>>>> b/drivers/iio/health/afe4403.c    
> >>>>>> index e9f87e42ff4f..a3507624b30f 100644
> >>>>>> --- a/drivers/iio/health/afe4403.c
> >>>>>> +++ b/drivers/iio/health/afe4403.c
> >>>>>> @@ -65,6 +65,7 @@ static const struct reg_field    
> >>> afe4403_reg_fields[]    
> >>>>> = {    
> >>>>>>   * @regulator: Pointer to the regulator for the IC
> >>>>>>   * @trig: IIO trigger for this device
> >>>>>>   * @irq: ADC_RDY line interrupt number
> >>>>>> + * @buffer: Used to construct data layout to push into IIO buffer.
> >>>>>>   */
> >>>>>>  struct afe4403_data {
> >>>>>>  	struct device *dev;
> >>>>>> @@ -74,6 +75,8 @@ struct afe4403_data {
> >>>>>>  	struct regulator *regulator;
> >>>>>>  	struct iio_trigger *trig;
> >>>>>>  	int irq;
> >>>>>> +	/* Ensure suitable alignment for timestamp */
> >>>>>> +	s32 buffer[8] __aligned(8);    
> >>>>>
> >>>>>
> >>>>> One of those fancy structs with the timestamp specified would be    
> >>> nice    
> >>>>> here like the other patches. IIRC we have 6 s32 channels, plus a s64
> >>>>> ts.    
> >>>>
> >>>> I think we may only have some of those channels enabled.  So ts may    
> >>> be it several    
> >>>> locations in the buffer. 
> >>>>     
> >>>
> >>> Might have been better to have the ts at the beginning, could have also
> >>> helped with alignment for when an odd number of channels are enabled.    
> >>
> >> Perhaps but for many use cases we don't turn timestamps on and we would need to pad
> >>  anyway for alignment of the fifo. 
> >>  
> >>>    
> >>>> Hence we could use the structure approach but it might give a false    
> >>> sense    
> >>>> of what is going on. 
> >>>>     
> >>>
> >>> That's true, it can always be cleaned later then.    
> >>
> >> Agreed. This is definitely something we can revisit sometime in the future.   
> > 
> > Formal Ack on applying the two afe patches?
> >   
> 
> Acked-by: Andrew F. Davis <afd@ti.com>
Thanks.

Applied to the fixes-togreg branch of iio.git and marked for stable.
These won't go anywhere now until after the merge window.

Thanks,

Jonathan

> 
> > Thanks,
> > 
> > Jonathan  
> >>
> >> J  
> >>>
> >>> Andrew
> >>>    
> >>>> J    
> >>>>>
> >>>>> Other than that everything looks good.
> >>>>>
> >>>>> Andrew
> >>>>>
> >>>>>    
> >>>>>>  };
> >>>>>>  
> >>>>>>  enum afe4403_chan_id {
> >>>>>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int    
> >>>>> irq, void *private)    
> >>>>>>  	struct iio_dev *indio_dev = pf->indio_dev;
> >>>>>>  	struct afe4403_data *afe = iio_priv(indio_dev);
> >>>>>>  	int ret, bit, i = 0;
> >>>>>> -	s32 buffer[8];
> >>>>>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
> >>>>>>  	u8 rx[3];
> >>>>>>  
> >>>>>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int    
> >>>>> irq, void *private)    
> >>>>>>  		if (ret)
> >>>>>>  			goto err;
> >>>>>>  
> >>>>>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
> >>>>>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	/* Disable reading from the device */
> >>>>>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int    
> >>>>> irq, void *private)    
> >>>>>>  	if (ret)
> >>>>>>  		goto err;
> >>>>>>  
> >>>>>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,    
> >>>>> pf->timestamp);    
> >>>>>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
> >>>>>> +					   pf->timestamp);
> >>>>>>  err:
> >>>>>>  	iio_trigger_notify_done(indio_dev->trig);
> >>>>>>  
> >>>>>>    
> >>>>     
> >>  
> >   

Re: [PATCH 08/11] iio:health:afe4404 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron]

On Sun, 17 May 2020 18:29:57 +0100
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 40 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 appart from previous readings.
> 
> Fixes: 87aec56e27ef ("iio: health: Add driver for the TI AFE4404 heart monitor")
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andrew F. Davis <afd@ti.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/health/afe4404.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/health/afe4404.c b/drivers/iio/health/afe4404.c
> index e728bbb21ca8..cebb1fd4d0b1 100644
> --- a/drivers/iio/health/afe4404.c
> +++ b/drivers/iio/health/afe4404.c
> @@ -83,6 +83,7 @@ static const struct reg_field afe4404_reg_fields[] = {
>   * @regulator: Pointer to the regulator for the IC
>   * @trig: IIO trigger for this device
>   * @irq: ADC_RDY line interrupt number
> + * @buffer: Used to construct a scan to push to the iio buffer.
>   */
>  struct afe4404_data {
>  	struct device *dev;
> @@ -91,6 +92,7 @@ struct afe4404_data {
>  	struct regulator *regulator;
>  	struct iio_trigger *trig;
>  	int irq;
> +	s32 buffer[10] __aligned(8);
>  };
>  
>  enum afe4404_chan_id {
> @@ -328,17 +330,17 @@ static irqreturn_t afe4404_trigger_handler(int irq, void *private)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct afe4404_data *afe = iio_priv(indio_dev);
>  	int ret, bit, i = 0;
> -	s32 buffer[10];
>  
>  	for_each_set_bit(bit, indio_dev->active_scan_mask,
>  			 indio_dev->masklength) {
>  		ret = regmap_read(afe->regmap, afe4404_channel_values[bit],
> -				  &buffer[i++]);
> +				  &afe->buffer[i++]);
>  		if (ret)
>  			goto err;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
> +					   pf->timestamp);
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);
>