IIO: Fused set 1 and 2 of timestamp alignment fixes

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/accel/kxsd9.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/iio/accel/kxsd9.c +11 -5

index 66b2e4cf24cf..0e18b92e2099 100644
@@ -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);
 

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Peter Meerwald <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/accel/mma8452.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/iio/accel/mma8452.c +8 -3

index ba27f8673131..1cf2b5db26ca 100644
@@ -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:

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Srinivas Pandruvada <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/iio/accel/bmc150-accel-core.c +13 -3

index 24864d9dfab5..48435865fdaf 100644
@@ -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;
 	}

Quoted 21 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

Quoted 5 lines

Fixes 3bbec9773389 ("iio: bmc150_accel: add support for hardware

fifo")

Reported-by Lars-Peter Clausen <[email protected]>

Cc Srinivas Pandruvada <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

Acked-by Srinivas Pandruvada <[email protected]>

Quoted 49 lines

---
 drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/iio/accel/bmc150-accel-core.c +13 -3

index 24864d9dfab5..48435865fdaf 100644
@@ -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;
 	}  

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/accel/mma7455_core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/iio/accel/mma7455_core.c +12 -4

index 7e99bcb3398d..922bd38ff6ea 100644
@@ -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:

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/gyro/itg3200_buffer.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/iio/gyro/itg3200_buffer.c +11 -4

index d3fbe9d86467..1c3c1bd53374 100644
@@ -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);
 

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Andreas Klinger <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/proximity/mb1232.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/iio/proximity/mb1232.c +9 -8

index 654564c45248..ad4b1fb2607a 100644
@@ -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);

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Narcisa Ana Maria Vasile <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/chemical/ccs811.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

drivers/iio/chemical/ccs811.c +9 -4

index 2b007e7568b2..60dd87e96f5f 100644
@@ -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:

From: Jonathan Cameron <[email protected]>

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.

Quoted 17 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

...

drivers/iio/light/si1145.c struct si1145_data { · line 183

Quoted 2 lines

drivers/iio/light/si1145.c +2 -0

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

Why can't we use proper structure here?

drivers/iio/light/si1145.c static irqreturn_t si1145_trigger_handler(int irq, void *private) · line 448

Quoted 5 lines

drivers/iio/light/si1145.c +0 -1

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

Quoted 27 lines · Andy Shevchenko

Quoted 17 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

...

drivers/iio/light/si1145.c struct si1145_data { · line 183

Quoted 2 lines

drivers/iio/light/si1145.c +2 -0

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

Why can't we use proper structure here?

Quoted 7 lines

drivers/iio/light/si1145.c static irqreturn_t si1145_trigger_handler(int irq, void *private) · line 448

Quoted 5 lines

drivers/iio/light/si1145.c +0 -1

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

Quoted 37 lines · On Wed, 22 Jul 2020 22:45:59 +0300 Andy Shevchenko <[email protected]>…

Quoted 34 lines · Andy Shevchenko

Quoted 17 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

...

drivers/iio/light/si1145.c struct si1145_data { · line 183

Quoted 2 lines

drivers/iio/light/si1145.c +2 -0

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

Why can't we use proper structure here?

drivers/iio/light/si1145.c static irqreturn_t si1145_trigger_handler(int irq, void *private) · line 448

Quoted 5 lines

drivers/iio/light/si1145.c +0 -1

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

Quoted 3 lines

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

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/light/max44000.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/iio/light/max44000.c +8 -4

index aa8ed1e3e89a..b8e721bced5b 100644
@@ -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;

From: Jonathan Cameron <[email protected]>

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.

Quoted 11 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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

Quoted 1 line

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

apart

Quoted 4 lines

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.

...

Quoted 2 lines

drivers/iio/light/rpr0521.c +2 -0

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

drivers/iio/light/rpr0521.c static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p) · line 474

Quoted 6 lines

drivers/iio/light/rpr0521.c +2 -1

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

Quoted 26 lines · On Wed, 22 Jul 2020 22:47:46 +0300 Andy Shevchenko <[email protected]>…

Quoted 11 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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

Quoted 1 line

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

apart

Quoted 4 lines

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.

...

Quoted 2 lines

drivers/iio/light/rpr0521.c +2 -0

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

Quoted 9 lines

drivers/iio/light/rpr0521.c static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p) · line 474

Quoted 6 lines

drivers/iio/light/rpr0521.c +2 -1

+               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

From: Jonathan Cameron <[email protected]>

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.

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/light/ltr501.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/iio/light/ltr501.c +9 -6

index 4bac0646398d..b4323d2db0b1 100644
@@ -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:

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Gregor Boirie <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Linus Walleij <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/magnetometer/ak8975.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/iio/magnetometer/ak8975.c +11 -5

index 03d71f796177..623766ff800b 100644
@@ -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:

From: Jonathan Cameron <[email protected]>

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.

(quoted diff) +2 -1

+               u8 temp;
 
-       u8 buffer[16]; /* 3 16-bit channels + 1 byte temp + padding + ts */
 
+               data->scan.temp = ret;

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

Cc Daniel Baluta <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/imu/bmi160/bmi160.h      | 2 ++
 drivers/iio/imu/bmi160/bmi160_core.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

drivers/iio/imu/bmi160/bmi160.h +2 -0

index a82e040bd109..d29f1b5d1658 100644
@@ -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;

drivers/iio/imu/bmi160/bmi160_core.c +2 -3

index 222ebb26f013..86cfd75ea125 100644
@@ -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;

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Lorenzo Bianconi <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

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

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h +6 -0

index d80ba2e688ed..9275346a9cc1 100644
@@ -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 = {

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c +20 -16

index 7de10bd636ea..9e0404e3581d 100644
@@ -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;

Hi Jonathan,

looks perfect for me.

Thanks for the fix,

JB

Reviewed-by Jean-Baptiste Maneyrol <[email protected]>

From: [email protected] <[email protected]> on behalf of Jonathan Cameron <[email protected]>
Sent: Wednesday, July 22, 2020 17:50
To: [email protected] <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Lars-Peter Clausen <[email protected]>; Peter Meerwald <[email protected]>; Jonathan Cameron <[email protected]>; Jean-Baptiste Maneyrol <[email protected]>
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 <[email protected]>

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 <[email protected]>

Cc Jean-Baptiste Maneyrol <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

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

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +0 -0

index cd38b3fccc7b..e4df2d51b689 100644
@@ -122,6 +122,9 @@ struct inv_mpu6050_chip_config {
          u8 user_ctrl;
  };

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +3 -0

+/* 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

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +2 -0

@@ -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*/

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +0 -0

@@ -334,9 +339,6 @@ struct inv_mpu6050_state {
  #define INV_ICM20608_TEMP_OFFSET             8170
  #define INV_ICM20608_TEMP_SCALE              3059976

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +0 -3

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

drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +0 -1

index b533fa2dad0a..d8e6b88ddffc 100644
@@ -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"

 /**

drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +3 -4

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

drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +2 -2

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

drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) · line 183

Quoted 11 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +2 -2

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

Hi Jonathan,

perfect.

Thanks for the fix,
JB

Reviewed-by Jean-Baptiste Maneyrol <[email protected]>

From: [email protected] <[email protected]> on behalf of Jonathan Cameron <[email protected]>
Sent: Wednesday, July 22, 2020 17:50
To: [email protected] <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Lars-Peter Clausen <[email protected]>; Peter Meerwald <[email protected]>; Jonathan Cameron <[email protected]>; Jean-Baptiste Maneyrol <[email protected]>
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 <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +2 -2

index d8e6b88ddffc..45c37525c2f1 100644
@@ -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 */

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.

Quoted 7 lines · Jonathan Cameron

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.

Quoted 10 lines · On Thu, 23 Jul 2020 15:17:16 +0300 Andy Shevchenko <[email protected]>…

Quoted 7 lines · Jonathan Cameron

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.

Quoted 3 lines

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

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/ti-adc081c.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/iio/adc/ti-adc081c.c +8 -3

index 9426f70a8005..cf63983a54d9 100644
@@ -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);

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

Cc Mårten Lindahl <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/iio/adc/ti-adc084s021.c +7 -3

index 9017e1e24273..dfba34834a57 100644
@@ -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);

From: Jonathan Cameron <[email protected]>

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 <[email protected]>

---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/iio/adc/ti-adc084s021.c +5 -5

index dfba34834a57..fb14b92fa6e7 100644
@@ -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;

Quoted 15 lines · On Wed, 22 Jul 2020 16:50:58 +0100 Jonathan Cameron <[email protected]> wrote:

From: Jonathan Cameron <[email protected]>

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

Quoted 1 line

Signed-off-by Jonathan Cameron <[email protected]>

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

Quoted 54 lines

---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/iio/adc/ti-adc084s021.c +5 -5

index dfba34834a57..fb14b92fa6e7 100644
@@ -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;  

Quoted 18 lines · On Sun, 9 Aug 2020 18:47:55 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 15 lines · On Wed, 22 Jul 2020 16:50:58 +0100 Jonathan Cameron <[email protected]> wrote:

From: Jonathan Cameron <[email protected]>

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

Quoted 5 lines

Quoted 1 line

Signed-off-by Jonathan Cameron <[email protected]>

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

Quoted 58 lines

Jonathan

Quoted 54 lines

---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/iio/adc/ti-adc084s021.c +5 -5

index dfba34834a57..fb14b92fa6e7 100644
@@ -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;    

Quoted 27 lines · On Sun, 13 Sep 2020 13:01:46 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 23 lines · On Sun, 9 Aug 2020 18:47:55 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 15 lines · On Wed, 22 Jul 2020 16:50:58 +0100 Jonathan Cameron <[email protected]> wrote:

From: Jonathan Cameron <[email protected]>

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

Quoted 1 line

Signed-off-by Jonathan Cameron <[email protected]>

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.

Quoted 2 lines

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.

Quoted 64 lines

Thanks,

Jonathan

Quoted 58 lines

Jonathan

Quoted 54 lines

---
 drivers/iio/adc/ti-adc084s021.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/iio/adc/ti-adc084s021.c +5 -5

index dfba34834a57..fb14b92fa6e7 100644
@@ -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;      

From: Jonathan Cameron <[email protected]>

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.

Quoted 16 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

...

drivers/iio/adc/ti-ads124s08.c struct ads124s_private { · line 106

Quoted 5 lines

drivers/iio/adc/ti-ads124s08.c +5 -0

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

u32 vs. u16?