[PATCH v3 00/10] iio: Fix unsafe buffer attributes

[PATCH v3 00/10] iio: Fix unsafe buffer attributes

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 3930 bytes --]

IIO The iio_triggered_buffer_setup_ext() has been changed to expect that
all attributes given in buffer_attrs array are device-attributes. This
expectation has not been forced by the API and not all existing users
were checked. Some drivers do register attributes created by
IIO_CONST_ATTR().

The added attribute "wrapping" does not copy the pointer to stored
string constant and when the sysfs file is read the kernel will access
to invalid address.

First 4 patches in the series aim to address both the drivers using
IIO_CONST_ATTR(). These patches should be backportable. Please note that
these patches have not been tested in the HW so testing/reviewing is
highly appreciated.

Following 4 (patches 5 ... 9) contain a minor simplification adding a
macro for omitting open-coding of dummy attribute show functions.

I am not super happy about the new macro IIO_STATIC_CONST_DEVICE_ATTR()
which unconditionally creates a static function and a static struct
iio_dev_attr. OTOH, I do believe static function + static struct
iio_dev_attr should be the right thing to do for majority of use cases.

The last patch in the series (patch 10) aims to change the function
signatures so that similar issues would be avoided in the future.

Here we also change the struct iio_buffer to have array of pointers to
iio_dev_attr in order to avoid yet another copying in side the
iio_triggered_buffer_setup_ext(). This change appeared to be somewhat
intrusive - and as I lack the hardware to do thorough testing I added
the request for testing tag here. Especially testing of adi-axi-adc
would be highly appreciated as it is using the
industrialio-buffer-dmaengine.

Changelog v3:
  - Split the series to allow easier backport and review. This also
    allows merging the smaller fixes with less of a risk for crashing
    and burning everything :)

Changelog v2:
  - fix also the cros_ec_sensors_core.c
  - fix also the industrialio-buffer-dmaengine.c
  - add RFT + this cover-letter.

--


Matti Vaittinen (10):
  iio: adxl367: Fix unsafe buffer attributes
  iio: adxl372: Fix unsafe buffer attributes
  iio: bmc150-accel-core: Fix unsafe buffer attributes
  iio: at91-sama5d2_adc: Fix unsafe buffer attributes
  iio: Add IIO_STATIC_CONST_DEVICE_ATTR
  iio: adxl367: Use IIO_STATIC_CONST_DEVICE_ATTR()
  iio: adxl372: Use IIO_STATIC_CONST_DEVICE_ATTR()
  iio: bmc150-accel-core: Use IIO_STATIC_CONST_DEVICE_ATTR()
  iio: at91-sama5d2_adc: Use IIO_STATIC_CONST_DEVICE_ATTR()
  iio: Don't silently expect attribute types

 drivers/iio/accel/adxl367.c                    | 16 ++++++++--------
 drivers/iio/accel/adxl372.c                    | 16 ++++++++--------
 drivers/iio/accel/bmc150-accel-core.c          | 18 +++++++++---------
 drivers/iio/adc/at91-sama5d2_adc.c             | 16 ++++++++--------
 .../iio/buffer/industrialio-buffer-dmaengine.c |  4 ++--
 .../iio/buffer/industrialio-triggered-buffer.c |  4 ++--
 drivers/iio/buffer/kfifo_buf.c                 |  2 +-
 .../cros_ec_sensors/cros_ec_sensors_core.c     |  6 +++---
 .../common/hid-sensors/hid-sensor-trigger.c    |  8 ++++----
 drivers/iio/industrialio-buffer.c              | 11 +++++++----
 include/linux/iio/buffer_impl.h                |  2 +-
 include/linux/iio/kfifo_buf.h                  |  3 ++-
 include/linux/iio/sysfs.h                      | 11 +++++++++++
 include/linux/iio/triggered_buffer.h           |  6 +++---
 14 files changed, 69 insertions(+), 54 deletions(-)


base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
-- 
2.37.3


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 04/10] iio: at91-sama5d2_adc: Fix unsafe buffer attributes

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 3059 bytes --]

The iio_triggered_buffer_setup_ext() was changed by
commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
to silently expect that all attributes given in buffer_attrs array are
device-attributes. This expectation was not forced by the API - and some
drivers did register attributes created by IIO_CONST_ATTR().

The added attribute "wrapping" does not copy the pointer to stored
string constant and when the sysfs file is read the kernel will access
to invalid location.

Change the IIO_CONST_ATTRs from the driver to IIO_DEVICE_ATTR in order
to prevent the invalid memory access.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Fixes: 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")

---

v2 => v3:
Split change to own patch for simpler fix backporting.
---
 drivers/iio/adc/at91-sama5d2_adc.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 279430c1d88c..6e3f9fa93cee 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1841,13 +1841,26 @@ static ssize_t at91_adc_get_watermark(struct device *dev,
 	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
 }
 
+static ssize_t hwfifo_watermark_min_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "2");
+}
+
+static ssize_t hwfifo_watermark_max_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", AT91_HWFIFO_MAX_SIZE_STR);
+}
+
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       at91_adc_get_fifo_state, NULL, 0);
 static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
 		       at91_adc_get_watermark, NULL, 0);
-
-static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
-static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_min, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
 		      __stringify(AT91_OSR_1SAMPLES) " "
@@ -1864,8 +1877,8 @@ static const struct attribute_group at91_adc_attribute_group = {
 };
 
 static const struct attribute *at91_adc_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
 	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
 	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
 	NULL,
-- 
2.37.3


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/10] iio: at91-sama5d2_adc: Fix unsafe buffer attributes

[Claudiu.Beznea]

On 03.10.2022 11:11, Matti Vaittinen wrote:
> The iio_triggered_buffer_setup_ext() was changed by
> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> to silently expect that all attributes given in buffer_attrs array are
> device-attributes. This expectation was not forced by the API - and some
> drivers did register attributes created by IIO_CONST_ATTR().
> 
> The added attribute "wrapping" does not copy the pointer to stored
> string constant and when the sysfs file is read the kernel will access
> to invalid location.
> 
> Change the IIO_CONST_ATTRs from the driver to IIO_DEVICE_ATTR in order
> to prevent the invalid memory access.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Fixes: 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")

Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>

on SAMA5D2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/10] iio: at91-sama5d2_adc: Fix unsafe buffer attributes

[Jonathan Cameron]

On Thu, 6 Oct 2022 08:34:17 +0000
<Claudiu.Beznea@microchip.com> wrote:

> On 03.10.2022 11:11, Matti Vaittinen wrote:
> > The iio_triggered_buffer_setup_ext() was changed by
> > commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> > to silently expect that all attributes given in buffer_attrs array are
> > device-attributes. This expectation was not forced by the API - and some
> > drivers did register attributes created by IIO_CONST_ATTR().
> > 
> > The added attribute "wrapping" does not copy the pointer to stored
> > string constant and when the sysfs file is read the kernel will access
> > to invalid location.
> > 
> > Change the IIO_CONST_ATTRs from the driver to IIO_DEVICE_ATTR in order
> > to prevent the invalid memory access.
> > 
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > Fixes: 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")  
> 
> Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> on SAMA5D2
> 
Applied to the fixes-togreg branch of iio.git and marked for stable.

For the reset of the series I'll need to wait for these first 4 patches to make their
way to upstream of the togreg branch then queue the rest up on top of that.

Jonathan

> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 09/10] iio: at91-sama5d2_adc: Use IIO_STATIC_CONST_DEVICE_ATTR()

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 1942 bytes --]

Slightly simplify by dropping open-coded constant data iio_dev_attr
functions and using the IIO_STATIC_CONST_DEVICE_ATTR() instead.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 6e3f9fa93cee..dca014d1108f 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1841,26 +1841,13 @@ static ssize_t at91_adc_get_watermark(struct device *dev,
 	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
 }
 
-static ssize_t hwfifo_watermark_min_show(struct device *dev,
-					 struct device_attribute *attr,
-					 char *buf)
-{
-	return sysfs_emit(buf, "%s\n", "2");
-}
-
-static ssize_t hwfifo_watermark_max_show(struct device *dev,
-					 struct device_attribute *attr,
-					 char *buf)
-{
-	return sysfs_emit(buf, "%s\n", AT91_HWFIFO_MAX_SIZE_STR);
-}
-
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       at91_adc_get_fifo_state, NULL, 0);
 static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
 		       at91_adc_get_watermark, NULL, 0);
-static IIO_DEVICE_ATTR_RO(hwfifo_watermark_min, 0);
-static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
+
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "2");
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
 		      __stringify(AT91_OSR_1SAMPLES) " "
-- 
2.37.3


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] iio: at91-sama5d2_adc: Use IIO_STATIC_CONST_DEVICE_ATTR()

[Claudiu.Beznea]

On 03.10.2022 11:13, Matti Vaittinen wrote:
> Slightly simplify by dropping open-coded constant data iio_dev_attr
> functions and using the IIO_STATIC_CONST_DEVICE_ATTR() instead.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>

on SAMA5D2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 15135 bytes --]

The iio_triggered_buffer_setup_ext() and the
devm_iio_kfifo_buffer_setup_ext() were changed by
commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
to silently expect that all attributes given in buffer_attrs array are
device-attributes. This expectation was not forced by the API - and some
drivers did register attributes created by IIO_CONST_ATTR().

When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
the pointer to stored string constant and when the sysfs file is read the
kernel will access to invalid location.

Change the function signatures to expect an array of iio_dev_attrs to
avoid similar errors in the future.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
v2 => v3:
split the driver fixes to separate patches for easier back port and
adjust the commit message accordinly.

v1 => v2:
fix also industrialio-buffer-dmaengine.c and cros_ec_sensors_core.c

The fix is only superficially tested by a ROHM/kionix KX022A driver.
Proper testing with real in-tree IIO stuff is _highly_ appreciated.
---
 drivers/iio/accel/adxl367.c                          | 10 +++++-----
 drivers/iio/accel/adxl372.c                          | 10 +++++-----
 drivers/iio/accel/bmc150-accel-core.c                | 12 ++++++------
 drivers/iio/adc/at91-sama5d2_adc.c                   | 12 ++++++------
 drivers/iio/buffer/industrialio-buffer-dmaengine.c   |  4 ++--
 drivers/iio/buffer/industrialio-triggered-buffer.c   |  4 ++--
 drivers/iio/buffer/kfifo_buf.c                       |  2 +-
 .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  6 +++---
 drivers/iio/common/hid-sensors/hid-sensor-trigger.c  |  8 ++++----
 drivers/iio/industrialio-buffer.c                    | 11 +++++++----
 include/linux/iio/buffer_impl.h                      |  2 +-
 include/linux/iio/kfifo_buf.h                        |  3 ++-
 include/linux/iio/triggered_buffer.h                 |  6 +++---
 13 files changed, 47 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index 47cddd4e98b2..0922ac0fad9e 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -1193,11 +1193,11 @@ static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       adxl367_get_fifo_enabled, NULL, 0);
 
-static const struct attribute *adxl367_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+static const struct iio_dev_attr *adxl367_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark_min,
+	&iio_dev_attr_hwfifo_watermark_max,
+	&iio_dev_attr_hwfifo_watermark,
+	&iio_dev_attr_hwfifo_enabled,
 	NULL,
 };
 
diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 90e1d726b9c5..c4193286eb05 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -1006,11 +1006,11 @@ static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       adxl372_get_fifo_enabled, NULL, 0);
 
-static const struct attribute *adxl372_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+static const struct iio_dev_attr *adxl372_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark_min,
+	&iio_dev_attr_hwfifo_watermark_max,
+	&iio_dev_attr_hwfifo_watermark,
+	&iio_dev_attr_hwfifo_enabled,
 	NULL,
 };
 
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index b4a077944896..110591804b4c 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -933,11 +933,11 @@ static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
 static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
 		       bmc150_accel_get_fifo_watermark, NULL, 0);
 
-static const struct attribute *bmc150_accel_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+static const struct iio_dev_attr *bmc150_accel_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark_min,
+	&iio_dev_attr_hwfifo_watermark_max,
+	&iio_dev_attr_hwfifo_watermark,
+	&iio_dev_attr_hwfifo_enabled,
 	NULL,
 };
 
@@ -1665,7 +1665,7 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 			    enum bmc150_type type, const char *name,
 			    bool block_supported)
 {
-	const struct attribute **fifo_attrs;
+	const struct iio_dev_attr **fifo_attrs;
 	struct bmc150_accel_data *data;
 	struct iio_dev *indio_dev;
 	int ret;
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index dca014d1108f..f994366b0778 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1863,11 +1863,11 @@ static const struct attribute_group at91_adc_attribute_group = {
 	.attrs = at91_adc_attributes,
 };
 
-static const struct attribute *at91_adc_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+static const struct iio_dev_attr *at91_adc_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark_min,
+	&iio_dev_attr_hwfifo_watermark_max,
+	&iio_dev_attr_hwfifo_watermark,
+	&iio_dev_attr_hwfifo_enabled,
 	NULL,
 };
 
@@ -1884,7 +1884,7 @@ static int at91_adc_buffer_and_trigger_init(struct device *dev,
 					    struct iio_dev *indio)
 {
 	struct at91_adc_state *st = iio_priv(indio);
-	const struct attribute **fifo_attrs;
+	const struct iio_dev_attr **fifo_attrs;
 	int ret;
 
 	if (st->selected_trig->hw_trig)
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index f744b62a636a..5f85ba38e6f6 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -142,8 +142,8 @@ static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
 static IIO_DEVICE_ATTR(length_align_bytes, 0444,
 		       iio_dmaengine_buffer_get_length_align, NULL, 0);
 
-static const struct attribute *iio_dmaengine_buffer_attrs[] = {
-	&iio_dev_attr_length_align_bytes.dev_attr.attr,
+static const struct iio_dev_attr *iio_dmaengine_buffer_attrs[] = {
+	&iio_dev_attr_length_align_bytes,
 	NULL,
 };
 
diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
index 8d4fc97d1005..c7671b1f5ead 100644
--- a/drivers/iio/buffer/industrialio-triggered-buffer.c
+++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
@@ -41,7 +41,7 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	irqreturn_t (*thread)(int irq, void *p),
 	enum iio_buffer_direction direction,
 	const struct iio_buffer_setup_ops *setup_ops,
-	const struct attribute **buffer_attrs)
+	const struct iio_dev_attr **buffer_attrs)
 {
 	struct iio_buffer *buffer;
 	int ret;
@@ -110,7 +110,7 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					irqreturn_t (*thread)(int irq, void *p),
 					enum iio_buffer_direction direction,
 					const struct iio_buffer_setup_ops *ops,
-					const struct attribute **buffer_attrs)
+					const struct iio_dev_attr **buffer_attrs)
 {
 	int ret;
 
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 35d8b4077376..05b285f0eb22 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -270,7 +270,7 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
 				    const struct iio_buffer_setup_ops *setup_ops,
-				    const struct attribute **buffer_attrs)
+				    const struct iio_dev_attr **buffer_attrs)
 {
 	struct iio_buffer *buffer;
 
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 05a28d353e34..943e9e14d1e9 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -172,9 +172,9 @@ static ssize_t hwfifo_watermark_max_show(struct device *dev,
 
 static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
 
-static const struct attribute *cros_ec_sensor_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
+static const struct iio_dev_attr *cros_ec_sensor_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_timeout,
+	&iio_dev_attr_hwfifo_watermark_max,
 	NULL,
 };
 
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 1151434038d4..ad8910e6ad59 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -75,9 +75,9 @@ static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       _hid_sensor_get_fifo_state, NULL, 0);
 
-static const struct attribute *hid_sensor_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+static const struct iio_dev_attr *hid_sensor_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_timeout,
+	&iio_dev_attr_hwfifo_enabled,
 	NULL,
 };
 
@@ -231,7 +231,7 @@ static const struct iio_trigger_ops hid_sensor_trigger_ops = {
 int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
 				struct hid_sensor_common *attrb)
 {
-	const struct attribute **fifo_attrs;
+	const struct iio_dev_attr **fifo_attrs;
 	int ret;
 	struct iio_trigger *trig;
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index acc2b6c05d57..cc7ebafae571 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1599,6 +1599,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_dev_attr *p;
+	const struct iio_dev_attr *id_attr;
 	struct attribute **attr;
 	int ret, i, attrn, scan_el_attrcount, buffer_attrcount;
 	const struct iio_chan_spec *channels;
@@ -1608,6 +1609,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		while (buffer->attrs[buffer_attrcount] != NULL)
 			buffer_attrcount++;
 	}
+	buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
 
 	scan_el_attrcount = 0;
 	INIT_LIST_HEAD(&buffer->buffer_attr_list);
@@ -1650,7 +1652,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		}
 	}
 
-	attrn = buffer_attrcount + scan_el_attrcount + ARRAY_SIZE(iio_buffer_attrs);
+	attrn = buffer_attrcount + scan_el_attrcount;
 	attr = kcalloc(attrn + 1, sizeof(*attr), GFP_KERNEL);
 	if (!attr) {
 		ret = -ENOMEM;
@@ -1665,10 +1667,11 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		attr[2] = &dev_attr_watermark_ro.attr;
 
 	if (buffer->attrs)
-		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
-		       sizeof(struct attribute *) * buffer_attrcount);
+		for (i = 0, id_attr = buffer->attrs[i];
+		     (id_attr = buffer->attrs[i]); i++)
+			attr[ARRAY_SIZE(iio_buffer_attrs) + i] =
+				(struct attribute *)&id_attr->dev_attr.attr;
 
-	buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
 	buffer->buffer_group.attrs = attr;
 
 	for (i = 0; i < buffer_attrcount; i++) {
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index e2ca8ea23e19..89c3fd7c29ca 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -123,7 +123,7 @@ struct iio_buffer {
 	struct attribute_group buffer_group;
 
 	/* @attrs: Standard attributes of the buffer. */
-	const struct attribute **attrs;
+	const struct iio_dev_attr **attrs;
 
 	/* @demux_bounce: Buffer for doing gather from incoming scan. */
 	void *demux_bounce;
diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
index 8a83fb58232d..22874da0c8be 100644
--- a/include/linux/iio/kfifo_buf.h
+++ b/include/linux/iio/kfifo_buf.h
@@ -5,6 +5,7 @@
 struct iio_buffer;
 struct iio_buffer_setup_ops;
 struct iio_dev;
+struct iio_dev_attr;
 struct device;
 
 struct iio_buffer *iio_kfifo_allocate(void);
@@ -13,7 +14,7 @@ void iio_kfifo_free(struct iio_buffer *r);
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
 				    const struct iio_buffer_setup_ops *setup_ops,
-				    const struct attribute **buffer_attrs);
+				    const struct iio_dev_attr **buffer_attrs);
 
 #define devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops)	\
 	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (setup_ops), NULL)
diff --git a/include/linux/iio/triggered_buffer.h b/include/linux/iio/triggered_buffer.h
index 7490b05fc5b2..29e1fe146879 100644
--- a/include/linux/iio/triggered_buffer.h
+++ b/include/linux/iio/triggered_buffer.h
@@ -5,8 +5,8 @@
 #include <linux/iio/buffer.h>
 #include <linux/interrupt.h>
 
-struct attribute;
 struct iio_dev;
+struct iio_dev_attr;
 struct iio_buffer_setup_ops;
 
 int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
@@ -14,7 +14,7 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	irqreturn_t (*thread)(int irq, void *p),
 	enum iio_buffer_direction direction,
 	const struct iio_buffer_setup_ops *setup_ops,
-	const struct attribute **buffer_attrs);
+	const struct iio_dev_attr **buffer_attrs);
 void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev);
 
 #define iio_triggered_buffer_setup(indio_dev, h, thread, setup_ops)		\
@@ -28,7 +28,7 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					irqreturn_t (*thread)(int irq, void *p),
 					enum iio_buffer_direction direction,
 					const struct iio_buffer_setup_ops *ops,
-					const struct attribute **buffer_attrs);
+					const struct iio_dev_attr **buffer_attrs);
 
 #define devm_iio_triggered_buffer_setup(dev, indio_dev, h, thread, setup_ops)	\
 	devm_iio_triggered_buffer_setup_ext((dev), (indio_dev), (h), (thread),	\
-- 
2.37.3


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Andy Shevchenko]

On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:
> The iio_triggered_buffer_setup_ext() and the
> devm_iio_kfifo_buffer_setup_ext() were changed by
> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> to silently expect that all attributes given in buffer_attrs array are
> device-attributes. This expectation was not forced by the API - and some
> drivers did register attributes created by IIO_CONST_ATTR().
> 
> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
> the pointer to stored string constant and when the sysfs file is read the
> kernel will access to invalid location.
> 
> Change the function signatures to expect an array of iio_dev_attrs to
> avoid similar errors in the future.

...


Wouldn't be better to split this on per driver basis or is it impossible?

>  drivers/iio/accel/adxl367.c                          | 10 +++++-----
>  drivers/iio/accel/adxl372.c                          | 10 +++++-----
>  drivers/iio/accel/bmc150-accel-core.c                | 12 ++++++------
>  drivers/iio/adc/at91-sama5d2_adc.c                   | 12 ++++++------
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c   |  4 ++--
>  drivers/iio/buffer/industrialio-triggered-buffer.c   |  4 ++--
>  drivers/iio/buffer/kfifo_buf.c                       |  2 +-
>  .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  6 +++---
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c  |  8 ++++----
>  drivers/iio/industrialio-buffer.c                    | 11 +++++++----
>  include/linux/iio/buffer_impl.h                      |  2 +-
>  include/linux/iio/kfifo_buf.h                        |  3 ++-
>  include/linux/iio/triggered_buffer.h                 |  6 +++---

...

>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct iio_dev_attr *p;

> +	const struct iio_dev_attr *id_attr;

I'm wondering if we may keep this upper, so "longer line goes first" rule would
be satisfied.

>  	struct attribute **attr;
>  	int ret, i, attrn, scan_el_attrcount, buffer_attrcount;
>  	const struct iio_chan_spec *channels;

...

> +		for (i = 0, id_attr = buffer->attrs[i];
> +		     (id_attr = buffer->attrs[i]); i++)

Not sure why we have additional parentheses...

> +			attr[ARRAY_SIZE(iio_buffer_attrs) + i] =
> +				(struct attribute *)&id_attr->dev_attr.attr;

...and explicit casting here. Isn't attr is already of a struct attribute?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Matti Vaittinen]

Hi Andy,

Thanks for taking the time to review :) Much appreciated.

On 10/3/22 11:43, Andy Shevchenko wrote:
> On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:
>> The iio_triggered_buffer_setup_ext() and the
>> devm_iio_kfifo_buffer_setup_ext() were changed by
>> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
>> to silently expect that all attributes given in buffer_attrs array are
>> device-attributes. This expectation was not forced by the API - and some
>> drivers did register attributes created by IIO_CONST_ATTR().
>>
>> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
>> the pointer to stored string constant and when the sysfs file is read the
>> kernel will access to invalid location.
>>
>> Change the function signatures to expect an array of iio_dev_attrs to
>> avoid similar errors in the future.
> 
> ...
> 
> 
> Wouldn't be better to split this on per driver basis or is it impossible?

We need to change the callers and function signatures in one patch so we 
don't break bisecting.

> 
>>   	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>>   	struct iio_dev_attr *p;
> 
>> +	const struct iio_dev_attr *id_attr;
> 
> I'm wondering if we may keep this upper, so "longer line goes first" rule would
> be satisfied.

Sure.

> 
>>   	struct attribute **attr;
>>   	int ret, i, attrn, scan_el_attrcount, buffer_attrcount;
>>   	const struct iio_chan_spec *channels;
> 
> ...
> 
>> +		for (i = 0, id_attr = buffer->attrs[i];
>> +		     (id_attr = buffer->attrs[i]); i++)
> 
> Not sure why we have additional parentheses...

Because gcc warns about the assignment and suggests adding parenthesis 
if we don't.

>> +			attr[ARRAY_SIZE(iio_buffer_attrs) + i] =
>> +				(struct attribute *)&id_attr->dev_attr.attr;
> 
> ...and explicit casting here. Isn't attr is already of a struct attribute?

I am glad you asked :)
This is one of the "things" I was not really happy about. Here we hide 
the fact that our array is full of pointers to _const_ data. If we don't 
cast the compiler points this out. Old code did the same thing but it 
did this by just doing a memcpy for the pointers - which I personally 
consider even worse as it gets really easy to miss this. The cast at 
least hints there is something slightly "fishy" going on.

My "gut feeling" about the correct fix is we should check if some 
attributes in the array (stored to the struct here) actually need to be 
modified later (which I doubt). If I was keen on betting I'd bet we 
could switch the struct definition to also contain pointers to const 
attributes. I am afraid this would mean quite a few more changes to the 
function signatures (changing struct attribute * to const struct 
attribute *) here and there - and possibly also require some changes to 
drivers. Thus I didn't even look at that option in the scope of this 
fix. It should probably be a separate refactoring series. But yes - this 
cast should catch attention as it did.

Yours,
	-- Matti Vaittinen

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Matti Vaittinen]

On 10/3/22 11:58, Matti Vaittinen wrote:
> Hi Andy,
> 
> Thanks for taking the time to review :) Much appreciated.
> 
> On 10/3/22 11:43, Andy Shevchenko wrote:
>> On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:
>>> The iio_triggered_buffer_setup_ext() and the
>>> devm_iio_kfifo_buffer_setup_ext() were changed by
>>> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into 
>>> iio_dev_attr")
>>> to silently expect that all attributes given in buffer_attrs array are
>>> device-attributes. This expectation was not forced by the API - and some
>>> drivers did register attributes created by IIO_CONST_ATTR().
>>>
>>> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
>>> the pointer to stored string constant and when the sysfs file is read 
>>> the
>>> kernel will access to invalid location.
>>>
>>> Change the function signatures to expect an array of iio_dev_attrs to
>>> avoid similar errors in the future.
>>
>> ...
>> 
>>> +            attr[ARRAY_SIZE(iio_buffer_attrs) + i] =
>>> +                (struct attribute *)&id_attr->dev_attr.attr;
>>
>> ...and explicit casting here. Isn't attr is already of a struct 
>> attribute?
> 
> I am glad you asked :)
> This is one of the "things" I was not really happy about. Here we hide 
> the fact that our array is full of pointers to _const_ data. If we don't 
> cast the compiler points this out. Old code did the same thing but it 
> did this by just doing a memcpy for the pointers - which I personally 
> consider even worse as it gets really easy to miss this. The cast at 
> least hints there is something slightly "fishy" going on.
> 
> My "gut feeling" about the correct fix is we should check if some 
> attributes in the array (stored to the struct here) actually need to be 
> modified later (which I doubt). If I was keen on betting I'd bet we 
> could switch the struct definition to also contain pointers to const 
> attributes. I am afraid this would mean quite a few more changes to the 
> function signatures (changing struct attribute * to const struct 
> attribute *) here and there - and possibly also require some changes to 
> drivers. Thus I didn't even look at that option in the scope of this 
> fix. It should probably be a separate refactoring series. But yes - this 
> cast should catch attention as it did.
> 

Actually, now that you pointed it out - do you think this would warrant 
a FIXME comment?

> Yours,
>      -- Matti Vaittinen
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Andy Shevchenko]

On Mon, Oct 03, 2022 at 12:02:56PM +0300, Matti Vaittinen wrote:
> On 10/3/22 11:58, Matti Vaittinen wrote:
> > On 10/3/22 11:43, Andy Shevchenko wrote:
> > > On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:

...

> > > > +            attr[ARRAY_SIZE(iio_buffer_attrs) + i] =
> > > > +                (struct attribute *)&id_attr->dev_attr.attr;
> > > 
> > > ...and explicit casting here. Isn't attr is already of a struct
> > > attribute?
> > 
> > I am glad you asked :)
> > This is one of the "things" I was not really happy about. Here we hide
> > the fact that our array is full of pointers to _const_ data. If we don't
> > cast the compiler points this out. Old code did the same thing but it
> > did this by just doing a memcpy for the pointers - which I personally
> > consider even worse as it gets really easy to miss this. The cast at
> > least hints there is something slightly "fishy" going on.
> > 
> > My "gut feeling" about the correct fix is we should check if some
> > attributes in the array (stored to the struct here) actually need to be
> > modified later (which I doubt). If I was keen on betting I'd bet we
> > could switch the struct definition to also contain pointers to const
> > attributes. I am afraid this would mean quite a few more changes to the
> > function signatures (changing struct attribute * to const struct
> > attribute *) here and there - and possibly also require some changes to
> > drivers. Thus I didn't even look at that option in the scope of this
> > fix. It should probably be a separate refactoring series. But yes - this
> > cast should catch attention as it did.
> > 
> 
> Actually, now that you pointed it out - do you think this would warrant a
> FIXME comment?

Makes sense to me, but I'm not a maintainer of IIO :-)

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Andy Shevchenko]

On Mon, Oct 03, 2022 at 11:58:35AM +0300, Matti Vaittinen wrote:
> On 10/3/22 11:43, Andy Shevchenko wrote:
> > On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:

...

> > > +		for (i = 0, id_attr = buffer->attrs[i];
> > > +		     (id_attr = buffer->attrs[i]); i++)
> > 
> > Not sure why we have additional parentheses...
> 
> Because gcc warns about the assignment and suggests adding parenthesis if we
> don't.

Ah, this is a condition, so that's why compiler wants to have a _result_ of
the assignment and not the ambiguous thingy.

Btw, have you considered to switch to in-loop iterator definitions as we do
in many other places? Also, it might make sense to introduce for_each_...
type of macro helper if the loop is used more than once.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Claudiu.Beznea]

On 03.10.2022 11:13, Matti Vaittinen wrote:
> The iio_triggered_buffer_setup_ext() and the
> devm_iio_kfifo_buffer_setup_ext() were changed by
> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> to silently expect that all attributes given in buffer_attrs array are
> device-attributes. This expectation was not forced by the API - and some
> drivers did register attributes created by IIO_CONST_ATTR().
> 
> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
> the pointer to stored string constant and when the sysfs file is read the
> kernel will access to invalid location.
> 
> Change the function signatures to expect an array of iio_dev_attrs to
> avoid similar errors in the future.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>

on SAMA5D2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Matti Vaittinen]

Hi Claudiu,

On 10/6/22 11:35, Claudiu.Beznea@microchip.com wrote:
> On 03.10.2022 11:13, Matti Vaittinen wrote:
>> The iio_triggered_buffer_setup_ext() and the
>> devm_iio_kfifo_buffer_setup_ext() were changed by
>> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
>> to silently expect that all attributes given in buffer_attrs array are
>> device-attributes. This expectation was not forced by the API - and some
>> drivers did register attributes created by IIO_CONST_ATTR().
>>
>> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
>> the pointer to stored string constant and when the sysfs file is read the
>> kernel will access to invalid location.
>>
>> Change the function signatures to expect an array of iio_dev_attrs to
>> avoid similar errors in the future.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> on SAMA5D2
> 

Thanks a ton for the testing! I do _really_ appreciate it :) I am now 
slightly more confident regarding the fix here - and a lot more 
confident that we do have an actual bug (as you explained in the reply 
to the first RFT) :)

Yours
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Jonathan Cameron]

On Thu, 6 Oct 2022 15:53:52 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Claudiu,
> 
> On 10/6/22 11:35, Claudiu.Beznea@microchip.com wrote:
> > On 03.10.2022 11:13, Matti Vaittinen wrote:  
> >> The iio_triggered_buffer_setup_ext() and the
> >> devm_iio_kfifo_buffer_setup_ext() were changed by
> >> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> >> to silently expect that all attributes given in buffer_attrs array are
> >> device-attributes. This expectation was not forced by the API - and some
> >> drivers did register attributes created by IIO_CONST_ATTR().
> >>
> >> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
> >> the pointer to stored string constant and when the sysfs file is read the
> >> kernel will access to invalid location.
> >>
> >> Change the function signatures to expect an array of iio_dev_attrs to
> >> avoid similar errors in the future.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> > 
> > Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> > 
> > on SAMA5D2
> >   
> 
> Thanks a ton for the testing! I do _really_ appreciate it :) I am now 
> slightly more confident regarding the fix here - and a lot more 
> confident that we do have an actual bug (as you explained in the reply 
> to the first RFT) :)

You analysis was sound, so I've long been convinced ;)

Anyhow, one more coming through...
AD4130 v9 patch had same issue and so will also need updating with this
patch if it lands before yours.

Other than that static macro being ugly (which I can't improve on!)
all looks good to me, but I'll let it sit a while longer. If nothing
else I want to rebase the fixes-togreg tree on rc1 before putting the first
part of this series on top of it then letting them soak in next for
a few days,

Thanks,

Jonathan


> 
> Yours
> 	-- Matti
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Matti Vaittinen]

On 10/9/22 20:38, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 15:53:52 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Claudiu,
>>
>> On 10/6/22 11:35, Claudiu.Beznea@microchip.com wrote:
>>> On 03.10.2022 11:13, Matti Vaittinen wrote:
>>>> The iio_triggered_buffer_setup_ext() and the
>>>> devm_iio_kfifo_buffer_setup_ext() were changed by
>>>> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
>>>> to silently expect that all attributes given in buffer_attrs array are
>>>> device-attributes. This expectation was not forced by the API - and some
>>>> drivers did register attributes created by IIO_CONST_ATTR().
>>>>
>>>> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
>>>> the pointer to stored string constant and when the sysfs file is read the
>>>> kernel will access to invalid location.
>>>>
>>>> Change the function signatures to expect an array of iio_dev_attrs to
>>>> avoid similar errors in the future.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>
>>> Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>
>>> on SAMA5D2
>>>    
>>
>> Thanks a ton for the testing! I do _really_ appreciate it :) I am now
>> slightly more confident regarding the fix here - and a lot more
>> confident that we do have an actual bug (as you explained in the reply
>> to the first RFT) :)
> 
> You analysis was sound, so I've long been convinced ;)
> 
> Anyhow, one more coming through...
> AD4130 v9 patch had same issue and so will also need updating with this
> patch if it lands before yours.
> 
> Other than that static macro being ugly (which I can't improve on!)
> all looks good to me, but I'll let it sit a while longer. If nothing
> else I want to rebase the fixes-togreg tree on rc1 before putting the first
> part of this series on top of it then letting them soak in next for
> a few days,

Thanks Jonathan.

Can you please ping me if you want me to rebase/rework the series? (I 
may combine this with the kx022a-series then, but naturally not all 
patches in the series need to be applied at once. Eg, fixes can be taken 
in faster, kx022a part can be iterated, iterated, iterated... ;] ).

Yours
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH v3 10/10] iio: Don't silently expect attribute types

[Jonathan Cameron]

On Mon, 10 Oct 2022 12:36:54 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 10/9/22 20:38, Jonathan Cameron wrote:
> > On Thu, 6 Oct 2022 15:53:52 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> Hi Claudiu,
> >>
> >> On 10/6/22 11:35, Claudiu.Beznea@microchip.com wrote:  
> >>> On 03.10.2022 11:13, Matti Vaittinen wrote:  
> >>>> The iio_triggered_buffer_setup_ext() and the
> >>>> devm_iio_kfifo_buffer_setup_ext() were changed by
> >>>> commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> >>>> to silently expect that all attributes given in buffer_attrs array are
> >>>> device-attributes. This expectation was not forced by the API - and some
> >>>> drivers did register attributes created by IIO_CONST_ATTR().
> >>>>
> >>>> When using IIO_CONST_ATTRs the added attribute "wrapping" does not copy
> >>>> the pointer to stored string constant and when the sysfs file is read the
> >>>> kernel will access to invalid location.
> >>>>
> >>>> Change the function signatures to expect an array of iio_dev_attrs to
> >>>> avoid similar errors in the future.
> >>>>
> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> >>>
> >>> Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >>>
> >>> on SAMA5D2
> >>>      
> >>
> >> Thanks a ton for the testing! I do _really_ appreciate it :) I am now
> >> slightly more confident regarding the fix here - and a lot more
> >> confident that we do have an actual bug (as you explained in the reply
> >> to the first RFT) :)  
> > 
> > You analysis was sound, so I've long been convinced ;)
> > 
> > Anyhow, one more coming through...
> > AD4130 v9 patch had same issue and so will also need updating with this
> > patch if it lands before yours.
> > 
> > Other than that static macro being ugly (which I can't improve on!)
> > all looks good to me, but I'll let it sit a while longer. If nothing
> > else I want to rebase the fixes-togreg tree on rc1 before putting the first
> > part of this series on top of it then letting them soak in next for
> > a few days,  
> 
> Thanks Jonathan.
> 
> Can you please ping me if you want me to rebase/rework the series? (I 
> may combine this with the kx022a-series then, but naturally not all 
> patches in the series need to be applied at once. Eg, fixes can be taken 
> in faster, kx022a part can be iterated, iterated, iterated... ;] ).

Applied the remainder of this series. As expected need to make the changes
in patch 10 to your kx022a driver and the ad4130 ADC that also crossed with
this series.

+CC Cosmin for the ad4130.  Please check the result in the
testing branch of iio.git.

Applied to the togreg branch of iio.git and pushed out initially as testing.

This is a nice hardening of the code against future mistakes.

Thanks,

Jonathan

> 
> Yours
> 	-- Matti
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel