All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment
@ 2021-05-01 17:13 Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp() Jonathan Cameron
                   ` (11 more replies)
  0 siblings, 12 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Eugen Hristev, Andreas Klinger, Matt Ranostay,
	Gwendal Grignou, Song Qiang, Mathieu Othacehe,
	Parthiban Nallathambi

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

I finally got around to do a manual audit of all the calls to
iio_push_to_buffers_with_timestamp() which has the somewhat odd requirements
of:
1. 8 byte alignment of the provided buffer.
2. space for an 8 byte naturally aligned timestamp to be inserted at the
   end.

Unfortunately there were rather a lot of these left, but time to bite the bullet
and clean them up.

As discussed previous in
https://lore.kernel.org/linux-iio/20200920112742.170751-1-jic23@kernel.org/
it is not easy to fix the alignment issue without requiring a bounce buffer
(see part 4 of the alignment fixes for a proposal for that where it is
absolutely necessary).

Part 3 contains the cases where the struct approach in part 2 did not seem
appropriate.  Normally there are two possible reasons for this.
1.  Would have required an additional memset operation to avoid any
    possibility of leaking kernel data.
2.  The location of the timestamp may depend on the channels enabled.
    This normally happens when the max sizeof channels is greater than
    8 bytes.

Once all cases are fixes, I'll take a look at hardening against any
accidental reintroductions. Note that on many platforms and usecases the
bug in question will never occur.

Cc: Eugen Hristev <eugen.hristev@microchip.com>
Cc: Andreas Klinger <ak@it-klinger.d>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Song Qiang <songqiang1304521@gmail.com>
Cc: Mathieu Othacehe <m.othacehe@gmail.com>
Cc: Parthiban Nallathambi <pn@denx.de>

Jonathan Cameron (11):
  iio: adc: at91-sama5d2: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()
  iio: adc: hx711: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()
  iio: adc: mxs-lradc: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()
  iio: adc: ti-ads8688: Fix alignment of buffer in
    iio_push_to_buffers_with_timestamp()
  iio: chemical: atlas: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()
  iio: cros_ec_sensors: Fix alignment of buffer in
    iio_push_to_buffers_with_timestamp()
  iio: potentiostat: lmp91000: Fix alignment of buffer in
    iio_push_to_buffers_with_timestamp()
  iio: magn: rm3100: Fix alignment of buffer in
    iio_push_to_buffers_with_timestamp()
  iio: light: vcnl4000: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()
  iio: light: vcnl4035: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()
  iio: prox: isl29501: Fix buffer alignment in
    iio_push_to_buffers_with_timestamp()

 drivers/iio/adc/at91-sama5d2_adc.c              | 3 ++-
 drivers/iio/adc/hx711.c                         | 4 ++--
 drivers/iio/adc/mxs-lradc-adc.c                 | 3 ++-
 drivers/iio/adc/ti-ads8688.c                    | 3 ++-
 drivers/iio/chemical/atlas-sensor.c             | 4 ++--
 drivers/iio/light/vcnl4000.c                    | 2 +-
 drivers/iio/light/vcnl4035.c                    | 3 ++-
 drivers/iio/magnetometer/rm3100-core.c          | 3 ++-
 drivers/iio/potentiostat/lmp91000.c             | 4 ++--
 drivers/iio/proximity/isl29501.c                | 2 +-
 include/linux/iio/common/cros_ec_sensors_core.h | 2 +-
 11 files changed, 19 insertions(+), 14 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-26 17:23   ` Eugen.Hristev
  2021-05-01 17:13 ` [PATCH 02/11] iio: adc: hx711: " Jonathan Cameron
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Eugen Hristev

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

To make code more readable, use a structure to express the channel
layout and ensure the timestamp is 8 byte aligned.

Found during an audit of all calls of this function.

Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index a7826f097b95..d356b515df09 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -403,7 +403,8 @@ struct at91_adc_state {
 	struct at91_adc_dma		dma_st;
 	struct at91_adc_touch		touch_st;
 	struct iio_dev			*indio_dev;
-	u16				buffer[AT91_BUFFER_MAX_HWORDS];
+	/* Ensure naturally aligned timestamp */
+	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
 	 * sysfs.
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 02/11] iio: adc: hx711: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp() Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 03/11] iio: adc: mxs-lradc: " Jonathan Cameron
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Andreas Klinger

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

To make code more readable, use a structure to express the channel
layout and ensure the timestamp is 8 byte aligned.

Found during an audit of all calls of this function.

Fixes: d3bf60450d47 ("iio: hx711: add triggered buffer support")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/adc/hx711.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
index 6a173531d355..f7ee856a6b8b 100644
--- a/drivers/iio/adc/hx711.c
+++ b/drivers/iio/adc/hx711.c
@@ -86,9 +86,9 @@ struct hx711_data {
 	struct mutex		lock;
 	/*
 	 * triggered buffer
-	 * 2x32-bit channel + 64-bit timestamp
+	 * 2x32-bit channel + 64-bit naturally aligned timestamp
 	 */
-	u32			buffer[4];
+	u32			buffer[4] __aligned(8);
 	/*
 	 * delay after a rising edge on SCK until the data is ready DOUT
 	 * this is dependent on the hx711 where the datasheet tells a
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 03/11] iio: adc: mxs-lradc: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp() Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 02/11] iio: adc: hx711: " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 04/11] iio: adc: ti-ads8688: Fix alignment of buffer " Jonathan Cameron
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Andreas Klinger

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

To make code more readable, use a structure to express the channel
layout and ensure the timestamp is 8 byte aligned.
Add a comment on why the buffer is the size it is as not immediately
obvious.

Found during an audit of all calls of this function.

Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/adc/mxs-lradc-adc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
index 30e29f44ebd2..c480cb489c1a 100644
--- a/drivers/iio/adc/mxs-lradc-adc.c
+++ b/drivers/iio/adc/mxs-lradc-adc.c
@@ -115,7 +115,8 @@ struct mxs_lradc_adc {
 	struct device		*dev;
 
 	void __iomem		*base;
-	u32			buffer[10];
+	/* Maximum of 8 channels + 8 byte ts */
+	u32			buffer[10] __aligned(8);
 	struct iio_trigger	*trig;
 	struct completion	completion;
 	spinlock_t		lock;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 04/11] iio: adc: ti-ads8688: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (2 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 03/11] iio: adc: mxs-lradc: " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment " Jonathan Cameron
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron

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

Add __aligned(8) to ensure the buffer passed to
iio_push_to_buffers_with_timestamp() is suitable for the naturally
aligned timestamp that will be inserted.

Fixes: f214ff521fb1 ("iio: ti-ads8688: Update buffer allocation for timestamps")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-ads8688.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
index 16bcb37eebb7..79c803537dc4 100644
--- a/drivers/iio/adc/ti-ads8688.c
+++ b/drivers/iio/adc/ti-ads8688.c
@@ -383,7 +383,8 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
-	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)];
+	/* Ensure naturally aligned timestamp */
+	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);
 	int i, j = 0;
 
 	for (i = 0; i < indio_dev->masklength; i++) {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (3 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 04/11] iio: adc: ti-ads8688: Fix alignment of buffer " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-06  6:44   ` Matt Ranostay
  2021-05-01 17:13 ` [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer " Jonathan Cameron
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Matt Ranostay

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

Variable location for the timestamp, so just use __aligned(8)
to ensure it is always possible to naturally align it.

Found during an audit of all calls of uses of
iio_push_to_buffers_with_timestamp()

Fixes tag is not accurate, but it will need manual backporting beyond
that point if anyone cares.

Fixes: 0d15190f53b4 ("iio: chemical: atlas-ph-sensor: rename atlas-ph-sensor to atlas-sensor")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/chemical/atlas-sensor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
index 56ba6c82b501..6795722c68b2 100644
--- a/drivers/iio/chemical/atlas-sensor.c
+++ b/drivers/iio/chemical/atlas-sensor.c
@@ -91,8 +91,8 @@ struct atlas_data {
 	struct regmap *regmap;
 	struct irq_work work;
 	unsigned int interrupt_enabled;
-
-	__be32 buffer[6]; /* 96-bit data + 32-bit pad + 64-bit timestamp */
+	/* 96-bit data + 32-bit pad + 64-bit timestamp */
+	__be32 buffer[6] __aligned(8);
 };
 
 static const struct regmap_config atlas_regmap_config = {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (4 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 23:58   ` Gwendal Grignou
  2021-05-01 17:13 ` [PATCH 07/11] iio: potentiostat: lmp91000: " Jonathan Cameron
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Gwendal Grignou

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

The samples buffer is passed to iio_push_to_buffers_with_timestamp()
which requires a buffer aligned to 8 bytes as it is assumed that
the timestamp will be naturally aligned if present.

Fixes tag is inaccurate but prior to that likely manual backporting needed.

Fixes: 5a0b8cb46624c ("iio: cros_ec: Move cros_ec_sensors_core.h in /include")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
---
 include/linux/iio/common/cros_ec_sensors_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 7ce8a8adad58..c582e1a14232 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -77,7 +77,7 @@ struct cros_ec_sensors_core_state {
 		u16 scale;
 	} calib[CROS_EC_SENSOR_MAX_AXIS];
 	s8 sign[CROS_EC_SENSOR_MAX_AXIS];
-	u8 samples[CROS_EC_SAMPLE_SIZE];
+	u8 samples[CROS_EC_SAMPLE_SIZE] __aligned(8);
 
 	int (*read_ec_sensors_data)(struct iio_dev *indio_dev,
 				    unsigned long scan_mask, s16 *data);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 07/11] iio: potentiostat: lmp91000: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (5 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-06  6:44   ` Matt Ranostay
  2021-05-01 17:13 ` [PATCH 08/11] iio: magn: rm3100: " Jonathan Cameron
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Matt Ranostay

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

Add __aligned(8) to ensure the buffer passed to
iio_push_to_buffers_with_timestamp() is suitable for the naturally
aligned timestamp that will be inserted.

Here structure is not used, because this buffer is also used
elsewhere in the driver.

Fixes: 67e17300dc1d ("iio: potentiostat: add LMP91000 support")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/potentiostat/lmp91000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 8a9c576616ee..ff39ba975da7 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -71,8 +71,8 @@ struct lmp91000_data {
 
 	struct completion completion;
 	u8 chan_select;
-
-	u32 buffer[4]; /* 64-bit data + 64-bit timestamp */
+	/* 64-bit data + 64-bit naturally aligned timestamp */
+	u32 buffer[4] __aligned(8);
 };
 
 static const struct iio_chan_spec lmp91000_channels[] = {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 08/11] iio: magn: rm3100: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (6 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 07/11] iio: potentiostat: lmp91000: " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 09/11] iio: light: vcnl4000: Fix buffer alignment " Jonathan Cameron
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Song Qiang

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

Add __aligned(8) to ensure the buffer passed to
iio_push_to_buffers_with_timestamp() is suitable for the naturally
aligned timestamp that will be inserted.

Here an explicit structure is not used, because this buffer is used in
a non-trivial way for data repacking.

Fixes: 121354b2eceb ("iio: magnetometer: Add driver support for PNI RM3100")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Song Qiang <songqiang1304521@gmail.com>
---
 drivers/iio/magnetometer/rm3100-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index dd811da9cb6d..934da20781bb 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -78,7 +78,8 @@ struct rm3100_data {
 	bool use_interrupt;
 	int conversion_time;
 	int scale;
-	u8 buffer[RM3100_SCAN_BYTES];
+	/* Ensure naturally aligned timestamp */
+	u8 buffer[RM3100_SCAN_BYTES] __aligned(8);
 	struct iio_trigger *drdy_trig;
 
 	/*
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 09/11] iio: light: vcnl4000: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (7 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 08/11] iio: magn: rm3100: " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 10/11] iio: light: vcnl4035: " Jonathan Cameron
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Mathieu Othacehe

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

Add __aligned(8) to ensure the buffer passed to
iio_push_to_buffers_with_timestamp() is suitable for the naturally
aligned timestamp that will be inserted.

Here an explicit structure is not used, because the holes would
necessitate the addition of an explict memset(), to avoid a kernel
data leak, making for a less minimal fix.

Found during an audit of all callers of iio_push_to_buffers_with_timestamp()

Fixes: 8fe78d5261e7 ("iio: vcnl4000: Add buffer support for VCNL4010/20.")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/light/vcnl4000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 2f7916f95689..3b5e27053ef2 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -910,7 +910,7 @@ static irqreturn_t vcnl4010_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct vcnl4000_data *data = iio_priv(indio_dev);
 	const unsigned long *active_scan_mask = indio_dev->active_scan_mask;
-	u16 buffer[8] = {0}; /* 1x16-bit + ts */
+	u16 buffer[8] __aligned(8) = {0}; /* 1x16-bit + naturally aligned ts */
 	bool data_read = false;
 	unsigned long isr;
 	int val = 0;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 10/11] iio: light: vcnl4035: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (8 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 09/11] iio: light: vcnl4000: Fix buffer alignment " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-01 17:13 ` [PATCH 11/11] iio: prox: isl29501: " Jonathan Cameron
  2021-05-13 17:58 ` [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Parthiban Nallathambi

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

Add __aligned(8) to ensure the buffer passed to
iio_push_to_buffers_with_timestamp() is suitable for the naturally
aligned timestamp that will be inserted.

Here an explicit structure is not used, because the holes would
necessitate the addition of an explict memset(), to avoid a potential
kernel data leak, making for a less minimal fix.

Fixes: 55707294c4eb ("iio: light: Add support for vishay vcnl40352")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Parthiban Nallathambi <pn@denx.de>
---
 drivers/iio/light/vcnl4035.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
index ae87740d9cef..bc0777411712 100644
--- a/drivers/iio/light/vcnl4035.c
+++ b/drivers/iio/light/vcnl4035.c
@@ -102,7 +102,8 @@ static irqreturn_t vcnl4035_trigger_consumer_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct vcnl4035_data *data = iio_priv(indio_dev);
-	u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)];
+	/* Ensure naturally aligned timestamp */
+	u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)]  __aligned(8);
 	int ret;
 
 	ret = regmap_read(data->regmap, VCNL4035_ALS_DATA, (int *)buffer);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 11/11] iio: prox: isl29501: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (9 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 10/11] iio: light: vcnl4035: " Jonathan Cameron
@ 2021-05-01 17:13 ` Jonathan Cameron
  2021-05-13 17:58 ` [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
  11 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-01 17:13 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Mathieu Othacehe

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

Add __aligned(8) to ensure the buffer passed to
iio_push_to_buffers_with_timestamp() is suitable for the naturally
aligned timestamp that will be inserted.

Here an explicit structure is not used, because the holes would
necessitate the addition of an explict memset(), to avoid a kernel
data leak, making for a less minimal fix.

Fixes: 1c28799257bc ("iio: light: isl29501: Add support for the ISL29501 ToF sensor.")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/proximity/isl29501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/isl29501.c b/drivers/iio/proximity/isl29501.c
index 90e76451c972..5b6ea783795d 100644
--- a/drivers/iio/proximity/isl29501.c
+++ b/drivers/iio/proximity/isl29501.c
@@ -938,7 +938,7 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct isl29501_private *isl29501 = iio_priv(indio_dev);
 	const unsigned long *active_mask = indio_dev->active_scan_mask;
-	u32 buffer[4] = {}; /* 1x16-bit + ts */
+	u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
 
 	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
 		isl29501_register_read(isl29501, REG_DISTANCE, buffer);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 ` [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer " Jonathan Cameron
@ 2021-05-01 23:58   ` Gwendal Grignou
  2021-05-13 18:03     ` Jonathan Cameron
  0 siblings, 1 reply; 23+ messages in thread
From: Gwendal Grignou @ 2021-05-01 23:58 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio, Jonathan Cameron

Fixes tag is correct up to kernel stable 4.18.
Before, the include file to fix is
drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h:
commit 974e6f02e27 ("iio: cros_ec_sensors_core: Add common functions
for the ChromeOS EC Sensor Hub.") present since kernel stable 4.10.

Reviewed-by: Gwendal Grignou <gwendal@chromium.org

On Sat, May 1, 2021 at 10:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The samples buffer is passed to iio_push_to_buffers_with_timestamp()
> which requires a buffer aligned to 8 bytes as it is assumed that
> the timestamp will be naturally aligned if present.
>
> Fixes tag is inaccurate but prior to that likely manual backporting needed.
>
> Fixes: 5a0b8cb46624c ("iio: cros_ec: Move cros_ec_sensors_core.h in /include")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> ---
>  include/linux/iio/common/cros_ec_sensors_core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 7ce8a8adad58..c582e1a14232 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -77,7 +77,7 @@ struct cros_ec_sensors_core_state {
>                 u16 scale;
>         } calib[CROS_EC_SENSOR_MAX_AXIS];
>         s8 sign[CROS_EC_SENSOR_MAX_AXIS];
> -       u8 samples[CROS_EC_SAMPLE_SIZE];
> +       u8 samples[CROS_EC_SAMPLE_SIZE] __aligned(8);
>
>         int (*read_ec_sensors_data)(struct iio_dev *indio_dev,
>                                     unsigned long scan_mask, s16 *data);
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 07/11] iio: potentiostat: lmp91000: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 ` [PATCH 07/11] iio: potentiostat: lmp91000: " Jonathan Cameron
@ 2021-05-06  6:44   ` Matt Ranostay
  2021-05-13 18:04     ` Jonathan Cameron
  0 siblings, 1 reply; 23+ messages in thread
From: Matt Ranostay @ 2021-05-06  6:44 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: open list:IIO SUBSYSTEM AND DRIVERS, Jonathan Cameron

On Sat, May 1, 2021 at 10:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Add __aligned(8) to ensure the buffer passed to
> iio_push_to_buffers_with_timestamp() is suitable for the naturally
> aligned timestamp that will be inserted.
>
> Here structure is not used, because this buffer is also used
> elsewhere in the driver.
>
> Fixes: 67e17300dc1d ("iio: potentiostat: add LMP91000 support")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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

> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/potentiostat/lmp91000.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
> index 8a9c576616ee..ff39ba975da7 100644
> --- a/drivers/iio/potentiostat/lmp91000.c
> +++ b/drivers/iio/potentiostat/lmp91000.c
> @@ -71,8 +71,8 @@ struct lmp91000_data {
>
>         struct completion completion;
>         u8 chan_select;
> -
> -       u32 buffer[4]; /* 64-bit data + 64-bit timestamp */
> +       /* 64-bit data + 64-bit naturally aligned timestamp */
> +       u32 buffer[4] __aligned(8);
>  };
>
>  static const struct iio_chan_spec lmp91000_channels[] = {
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 ` [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment " Jonathan Cameron
@ 2021-05-06  6:44   ` Matt Ranostay
  2021-05-13 17:59     ` Jonathan Cameron
  0 siblings, 1 reply; 23+ messages in thread
From: Matt Ranostay @ 2021-05-06  6:44 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: open list:IIO SUBSYSTEM AND DRIVERS, Jonathan Cameron

On Sat, May 1, 2021 at 10:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Variable location for the timestamp, so just use __aligned(8)
> to ensure it is always possible to naturally align it.
>
> Found during an audit of all calls of uses of
> iio_push_to_buffers_with_timestamp()
>
> Fixes tag is not accurate, but it will need manual backporting beyond
> that point if anyone cares.
>
> Fixes: 0d15190f53b4 ("iio: chemical: atlas-ph-sensor: rename atlas-ph-sensor to atlas-sensor")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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

> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/chemical/atlas-sensor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> index 56ba6c82b501..6795722c68b2 100644
> --- a/drivers/iio/chemical/atlas-sensor.c
> +++ b/drivers/iio/chemical/atlas-sensor.c
> @@ -91,8 +91,8 @@ struct atlas_data {
>         struct regmap *regmap;
>         struct irq_work work;
>         unsigned int interrupt_enabled;
> -
> -       __be32 buffer[6]; /* 96-bit data + 32-bit pad + 64-bit timestamp */
> +       /* 96-bit data + 32-bit pad + 64-bit timestamp */
> +       __be32 buffer[6] __aligned(8);
>  };
>
>  static const struct regmap_config atlas_regmap_config = {
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment
  2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
                   ` (10 preceding siblings ...)
  2021-05-01 17:13 ` [PATCH 11/11] iio: prox: isl29501: " Jonathan Cameron
@ 2021-05-13 17:58 ` Jonathan Cameron
  2021-05-26 17:09   ` Jonathan Cameron
  11 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-13 17:58 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Eugen Hristev, Andreas Klinger, Matt Ranostay,
	Gwendal Grignou, Song Qiang, Mathieu Othacehe,
	Parthiban Nallathambi, Andy Shevchenko

On Sat,  1 May 2021 18:13:41 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

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

Thanks to those who have already provided reviews on this set.
If anyone has time to do a quick sanity check of the remaining patches
it would be much appreciated.  I'll pick up the ones which have been reviewed
in a few mins.

+Cc Andy who was kind enough to look at the other 'parts' of this mega series.

Thanks

Jonathan

> 
> I finally got around to do a manual audit of all the calls to
> iio_push_to_buffers_with_timestamp() which has the somewhat odd requirements
> of:
> 1. 8 byte alignment of the provided buffer.
> 2. space for an 8 byte naturally aligned timestamp to be inserted at the
>    end.
> 
> Unfortunately there were rather a lot of these left, but time to bite the bullet
> and clean them up.
> 
> As discussed previous in
> https://lore.kernel.org/linux-iio/20200920112742.170751-1-jic23@kernel.org/
> it is not easy to fix the alignment issue without requiring a bounce buffer
> (see part 4 of the alignment fixes for a proposal for that where it is
> absolutely necessary).
> 
> Part 3 contains the cases where the struct approach in part 2 did not seem
> appropriate.  Normally there are two possible reasons for this.
> 1.  Would have required an additional memset operation to avoid any
>     possibility of leaking kernel data.
> 2.  The location of the timestamp may depend on the channels enabled.
>     This normally happens when the max sizeof channels is greater than
>     8 bytes.
> 
> Once all cases are fixes, I'll take a look at hardening against any
> accidental reintroductions. Note that on many platforms and usecases the
> bug in question will never occur.
> 
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> Cc: Andreas Klinger <ak@it-klinger.d>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Song Qiang <songqiang1304521@gmail.com>
> Cc: Mathieu Othacehe <m.othacehe@gmail.com>
> Cc: Parthiban Nallathambi <pn@denx.de>
> 
> Jonathan Cameron (11):
>   iio: adc: at91-sama5d2: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
>   iio: adc: hx711: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
>   iio: adc: mxs-lradc: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
>   iio: adc: ti-ads8688: Fix alignment of buffer in
>     iio_push_to_buffers_with_timestamp()
>   iio: chemical: atlas: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
>   iio: cros_ec_sensors: Fix alignment of buffer in
>     iio_push_to_buffers_with_timestamp()
>   iio: potentiostat: lmp91000: Fix alignment of buffer in
>     iio_push_to_buffers_with_timestamp()
>   iio: magn: rm3100: Fix alignment of buffer in
>     iio_push_to_buffers_with_timestamp()
>   iio: light: vcnl4000: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
>   iio: light: vcnl4035: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
>   iio: prox: isl29501: Fix buffer alignment in
>     iio_push_to_buffers_with_timestamp()
> 
>  drivers/iio/adc/at91-sama5d2_adc.c              | 3 ++-
>  drivers/iio/adc/hx711.c                         | 4 ++--
>  drivers/iio/adc/mxs-lradc-adc.c                 | 3 ++-
>  drivers/iio/adc/ti-ads8688.c                    | 3 ++-
>  drivers/iio/chemical/atlas-sensor.c             | 4 ++--
>  drivers/iio/light/vcnl4000.c                    | 2 +-
>  drivers/iio/light/vcnl4035.c                    | 3 ++-
>  drivers/iio/magnetometer/rm3100-core.c          | 3 ++-
>  drivers/iio/potentiostat/lmp91000.c             | 4 ++--
>  drivers/iio/proximity/isl29501.c                | 2 +-
>  include/linux/iio/common/cros_ec_sensors_core.h | 2 +-
>  11 files changed, 19 insertions(+), 14 deletions(-)
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-06  6:44   ` Matt Ranostay
@ 2021-05-13 17:59     ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-13 17:59 UTC (permalink / raw)
  To: Matt Ranostay; +Cc: open list:IIO SUBSYSTEM AND DRIVERS, Jonathan Cameron

On Wed, 5 May 2021 23:44:54 -0700
Matt Ranostay <matt.ranostay@konsulko.com> wrote:

> On Sat, May 1, 2021 at 10:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Variable location for the timestamp, so just use __aligned(8)
> > to ensure it is always possible to naturally align it.
> >
> > Found during an audit of all calls of uses of
> > iio_push_to_buffers_with_timestamp()
> >
> > Fixes tag is not accurate, but it will need manual backporting beyond
> > that point if anyone cares.
> >
> > Fixes: 0d15190f53b4 ("iio: chemical: atlas-ph-sensor: rename atlas-ph-sensor to atlas-sensor")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to have a go at breaking things.

Thanks,

Jonathan

> 
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/iio/chemical/atlas-sensor.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> > index 56ba6c82b501..6795722c68b2 100644
> > --- a/drivers/iio/chemical/atlas-sensor.c
> > +++ b/drivers/iio/chemical/atlas-sensor.c
> > @@ -91,8 +91,8 @@ struct atlas_data {
> >         struct regmap *regmap;
> >         struct irq_work work;
> >         unsigned int interrupt_enabled;
> > -
> > -       __be32 buffer[6]; /* 96-bit data + 32-bit pad + 64-bit timestamp */
> > +       /* 96-bit data + 32-bit pad + 64-bit timestamp */
> > +       __be32 buffer[6] __aligned(8);
> >  };
> >
> >  static const struct regmap_config atlas_regmap_config = {
> > --
> > 2.31.1
> >  


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-01 23:58   ` Gwendal Grignou
@ 2021-05-13 18:03     ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-13 18:03 UTC (permalink / raw)
  To: Gwendal Grignou; +Cc: linux-iio, Jonathan Cameron

On Sat, 1 May 2021 16:58:21 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> Fixes tag is correct up to kernel stable 4.18.
> Before, the include file to fix is
> drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h:
> commit 974e6f02e27 ("iio: cros_ec_sensors_core: Add common functions
> for the ChromeOS EC Sensor Hub.") present since kernel stable 4.10.
> 
Applied with this extra info to the togreg branch of iio.git and pushed
out as testing for the autobuilders to poke at it.

This series (in it's complete form) is large enough that I'm not comfortable
rushing it in.  The bug is also rarely seen in practice so this can wait
for the next merge window.

Thanks,

Jonathan

> Reviewed-by: Gwendal Grignou <gwendal@chromium.org
> 
> On Sat, May 1, 2021 at 10:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > The samples buffer is passed to iio_push_to_buffers_with_timestamp()
> > which requires a buffer aligned to 8 bytes as it is assumed that
> > the timestamp will be naturally aligned if present.
> >
> > Fixes tag is inaccurate but prior to that likely manual backporting needed.
> >
> > Fixes: 5a0b8cb46624c ("iio: cros_ec: Move cros_ec_sensors_core.h in /include")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > ---
> >  include/linux/iio/common/cros_ec_sensors_core.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> > index 7ce8a8adad58..c582e1a14232 100644
> > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > @@ -77,7 +77,7 @@ struct cros_ec_sensors_core_state {
> >                 u16 scale;
> >         } calib[CROS_EC_SENSOR_MAX_AXIS];
> >         s8 sign[CROS_EC_SENSOR_MAX_AXIS];
> > -       u8 samples[CROS_EC_SAMPLE_SIZE];
> > +       u8 samples[CROS_EC_SAMPLE_SIZE] __aligned(8);
> >
> >         int (*read_ec_sensors_data)(struct iio_dev *indio_dev,
> >                                     unsigned long scan_mask, s16 *data);
> > --
> > 2.31.1
> >  


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 07/11] iio: potentiostat: lmp91000: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()
  2021-05-06  6:44   ` Matt Ranostay
@ 2021-05-13 18:04     ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-13 18:04 UTC (permalink / raw)
  To: Matt Ranostay; +Cc: open list:IIO SUBSYSTEM AND DRIVERS, Jonathan Cameron

On Wed, 5 May 2021 23:44:23 -0700
Matt Ranostay <matt.ranostay@konsulko.com> wrote:

> On Sat, May 1, 2021 at 10:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Add __aligned(8) to ensure the buffer passed to
> > iio_push_to_buffers_with_timestamp() is suitable for the naturally
> > aligned timestamp that will be inserted.
> >
> > Here structure is not used, because this buffer is also used
> > elsewhere in the driver.
> >
> > Fixes: 67e17300dc1d ("iio: potentiostat: add LMP91000 support")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

Thanks Matt.

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to see if they can break it.

Jonathan
> 
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/iio/potentiostat/lmp91000.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
> > index 8a9c576616ee..ff39ba975da7 100644
> > --- a/drivers/iio/potentiostat/lmp91000.c
> > +++ b/drivers/iio/potentiostat/lmp91000.c
> > @@ -71,8 +71,8 @@ struct lmp91000_data {
> >
> >         struct completion completion;
> >         u8 chan_select;
> > -
> > -       u32 buffer[4]; /* 64-bit data + 64-bit timestamp */
> > +       /* 64-bit data + 64-bit naturally aligned timestamp */
> > +       u32 buffer[4] __aligned(8);
> >  };
> >
> >  static const struct iio_chan_spec lmp91000_channels[] = {
> > --
> > 2.31.1
> >  


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment
  2021-05-13 17:58 ` [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
@ 2021-05-26 17:09   ` Jonathan Cameron
  2021-05-26 17:17     ` Andy Shevchenko
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-26 17:09 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Eugen Hristev, Andreas Klinger, Matt Ranostay,
	Gwendal Grignou, Song Qiang, Mathieu Othacehe,
	Parthiban Nallathambi, Andy Shevchenko

On Thu, 13 May 2021 18:58:32 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat,  1 May 2021 18:13:41 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Thanks to those who have already provided reviews on this set.
> If anyone has time to do a quick sanity check of the remaining patches
> it would be much appreciated.  I'll pick up the ones which have been reviewed
> in a few mins.
> 
> +Cc Andy who was kind enough to look at the other 'parts' of this mega series.

Anyone bored?  Still looking for a sanity check of the rest of this
series before I apply it.

I aim to revisit part 4 in the next few days.

Jonathan

> 
> Thanks
> 
> Jonathan
> 
> > 
> > I finally got around to do a manual audit of all the calls to
> > iio_push_to_buffers_with_timestamp() which has the somewhat odd requirements
> > of:
> > 1. 8 byte alignment of the provided buffer.
> > 2. space for an 8 byte naturally aligned timestamp to be inserted at the
> >    end.
> > 
> > Unfortunately there were rather a lot of these left, but time to bite the bullet
> > and clean them up.
> > 
> > As discussed previous in
> > https://lore.kernel.org/linux-iio/20200920112742.170751-1-jic23@kernel.org/
> > it is not easy to fix the alignment issue without requiring a bounce buffer
> > (see part 4 of the alignment fixes for a proposal for that where it is
> > absolutely necessary).
> > 
> > Part 3 contains the cases where the struct approach in part 2 did not seem
> > appropriate.  Normally there are two possible reasons for this.
> > 1.  Would have required an additional memset operation to avoid any
> >     possibility of leaking kernel data.
> > 2.  The location of the timestamp may depend on the channels enabled.
> >     This normally happens when the max sizeof channels is greater than
> >     8 bytes.
> > 
> > Once all cases are fixes, I'll take a look at hardening against any
> > accidental reintroductions. Note that on many platforms and usecases the
> > bug in question will never occur.
> > 
> > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > Cc: Andreas Klinger <ak@it-klinger.d>
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Song Qiang <songqiang1304521@gmail.com>
> > Cc: Mathieu Othacehe <m.othacehe@gmail.com>
> > Cc: Parthiban Nallathambi <pn@denx.de>
> > 
> > Jonathan Cameron (11):
> >   iio: adc: at91-sama5d2: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: adc: hx711: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: adc: mxs-lradc: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: adc: ti-ads8688: Fix alignment of buffer in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: chemical: atlas: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: cros_ec_sensors: Fix alignment of buffer in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: potentiostat: lmp91000: Fix alignment of buffer in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: magn: rm3100: Fix alignment of buffer in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: light: vcnl4000: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: light: vcnl4035: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> >   iio: prox: isl29501: Fix buffer alignment in
> >     iio_push_to_buffers_with_timestamp()
> > 
> >  drivers/iio/adc/at91-sama5d2_adc.c              | 3 ++-
> >  drivers/iio/adc/hx711.c                         | 4 ++--
> >  drivers/iio/adc/mxs-lradc-adc.c                 | 3 ++-
> >  drivers/iio/adc/ti-ads8688.c                    | 3 ++-
> >  drivers/iio/chemical/atlas-sensor.c             | 4 ++--
> >  drivers/iio/light/vcnl4000.c                    | 2 +-
> >  drivers/iio/light/vcnl4035.c                    | 3 ++-
> >  drivers/iio/magnetometer/rm3100-core.c          | 3 ++-
> >  drivers/iio/potentiostat/lmp91000.c             | 4 ++--
> >  drivers/iio/proximity/isl29501.c                | 2 +-
> >  include/linux/iio/common/cros_ec_sensors_core.h | 2 +-
> >  11 files changed, 19 insertions(+), 14 deletions(-)
> >   
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment
  2021-05-26 17:09   ` Jonathan Cameron
@ 2021-05-26 17:17     ` Andy Shevchenko
  0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2021-05-26 17:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, Jonathan Cameron, Eugen Hristev, Andreas Klinger,
	Matt Ranostay, Gwendal Grignou, Song Qiang, Mathieu Othacehe,
	Parthiban Nallathambi

On Wed, May 26, 2021 at 8:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 13 May 2021 18:58:32 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Sat,  1 May 2021 18:13:41 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Thanks to those who have already provided reviews on this set.
> > If anyone has time to do a quick sanity check of the remaining patches
> > it would be much appreciated.  I'll pick up the ones which have been reviewed
> > in a few mins.
> >
> > +Cc Andy who was kind enough to look at the other 'parts' of this mega series.
>
> Anyone bored?  Still looking for a sanity check of the rest of this
> series before I apply it.

Sorry, I have some high priority tasks, and this one is not in the list :-(

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-01 17:13 ` [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp() Jonathan Cameron
@ 2021-05-26 17:23   ` Eugen.Hristev
  2021-05-27  8:48     ` Jonathan Cameron
  0 siblings, 1 reply; 23+ messages in thread
From: Eugen.Hristev @ 2021-05-26 17:23 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jonathan.Cameron

On 5/1/21 8:13 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> To make code more readable, use a structure to express the channel
> layout and ensure the timestamp is 8 byte aligned.
> 
> Found during an audit of all calls of this function.
> 
> Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>   drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index a7826f097b95..d356b515df09 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -403,7 +403,8 @@ struct at91_adc_state {
>          struct at91_adc_dma             dma_st;
>          struct at91_adc_touch           touch_st;
>          struct iio_dev                  *indio_dev;
> -       u16                             buffer[AT91_BUFFER_MAX_HWORDS];
> +       /* Ensure naturally aligned timestamp */
> +       u16                             buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);

Hello Jonathan,

I am preparing to change this buffer to a dynamically allocated 
buffer... because we want to support several versions of the ADC with 
this driver, having an arbitrary number of channels..

You think it's possible to have this alignment when I move to a 
devm_kzalloc call ?

Thanks,
Eugen

>          /*
>           * lock to prevent concurrent 'single conversion' requests through
>           * sysfs.
> --
> 2.31.1
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp()
  2021-05-26 17:23   ` Eugen.Hristev
@ 2021-05-27  8:48     ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-05-27  8:48 UTC (permalink / raw)
  To: Eugen.Hristev; +Cc: jic23, linux-iio

On Wed, 26 May 2021 17:23:11 +0000
<Eugen.Hristev@microchip.com> wrote:

> On 5/1/21 8:13 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > To make code more readable, use a structure to express the channel
> > layout and ensure the timestamp is 8 byte aligned.
> > 
> > Found during an audit of all calls of this function.
> > 
> > Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > ---
> >   drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > index a7826f097b95..d356b515df09 100644
> > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > @@ -403,7 +403,8 @@ struct at91_adc_state {
> >          struct at91_adc_dma             dma_st;
> >          struct at91_adc_touch           touch_st;
> >          struct iio_dev                  *indio_dev;
> > -       u16                             buffer[AT91_BUFFER_MAX_HWORDS];
> > +       /* Ensure naturally aligned timestamp */
> > +       u16                             buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);  
> 
> Hello Jonathan,
> 
> I am preparing to change this buffer to a dynamically allocated 
> buffer... because we want to support several versions of the ADC with 
> this driver, having an arbitrary number of channels..
> 
> You think it's possible to have this alignment when I move to a 
> devm_kzalloc call ?

Hmm. You should be fine, but looking through the docs, guaranteed alignment for
devm_kmalloc is unsigned long long.  Sounds worryingly vague, but c99 (which kernel
uses) guarantees at least 64 bits so that's fine.

However, unless your maximum size is very large, I'd be cynical and just leave it
where it is.  Chances are it won't actually make any difference to the real amount
of memory allocated for at91_adc_state and it'll make your code simpler.

No one minds a buffer being too big :)

Jonathan

> 
> Thanks,
> Eugen
> 
> >          /*
> >           * lock to prevent concurrent 'single conversion' requests through
> >           * sysfs.
> > --
> > 2.31.1
> >   
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-05-27  8:50 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 17:13 [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
2021-05-01 17:13 ` [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp() Jonathan Cameron
2021-05-26 17:23   ` Eugen.Hristev
2021-05-27  8:48     ` Jonathan Cameron
2021-05-01 17:13 ` [PATCH 02/11] iio: adc: hx711: " Jonathan Cameron
2021-05-01 17:13 ` [PATCH 03/11] iio: adc: mxs-lradc: " Jonathan Cameron
2021-05-01 17:13 ` [PATCH 04/11] iio: adc: ti-ads8688: Fix alignment of buffer " Jonathan Cameron
2021-05-01 17:13 ` [PATCH 05/11] iio: chemical: atlas: Fix buffer alignment " Jonathan Cameron
2021-05-06  6:44   ` Matt Ranostay
2021-05-13 17:59     ` Jonathan Cameron
2021-05-01 17:13 ` [PATCH 06/11] iio: cros_ec_sensors: Fix alignment of buffer " Jonathan Cameron
2021-05-01 23:58   ` Gwendal Grignou
2021-05-13 18:03     ` Jonathan Cameron
2021-05-01 17:13 ` [PATCH 07/11] iio: potentiostat: lmp91000: " Jonathan Cameron
2021-05-06  6:44   ` Matt Ranostay
2021-05-13 18:04     ` Jonathan Cameron
2021-05-01 17:13 ` [PATCH 08/11] iio: magn: rm3100: " Jonathan Cameron
2021-05-01 17:13 ` [PATCH 09/11] iio: light: vcnl4000: Fix buffer alignment " Jonathan Cameron
2021-05-01 17:13 ` [PATCH 10/11] iio: light: vcnl4035: " Jonathan Cameron
2021-05-01 17:13 ` [PATCH 11/11] iio: prox: isl29501: " Jonathan Cameron
2021-05-13 17:58 ` [PATCH 00/11] IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment Jonathan Cameron
2021-05-26 17:09   ` Jonathan Cameron
2021-05-26 17:17     ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.