iio: Fix unsafe buffer attributes

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

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/accel/adxl367.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

drivers/iio/accel/adxl367.c +18 -5

index 47feb375b70b..7c7d78040793 100644
@@ -1185,17 +1185,30 @@ static ssize_t adxl367_get_fifo_watermark(struct device *dev,
 	return sysfs_emit(buf, "%d\n", fifo_watermark);
 }
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(ADXL367_FIFO_MAX_WATERMARK));
+static ssize_t hwfifo_watermark_min_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "1");
+}
+
+static ssize_t hwfifo_watermark_max_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", __stringify(ADXL367_FIFO_MAX_WATERMARK));
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_min, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
 static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
 		       adxl367_get_fifo_watermark, NULL, 0);
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       adxl367_get_fifo_enabled, NULL, 0);
 
 static const struct attribute *adxl367_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,

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

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/accel/adxl372.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

drivers/iio/accel/adxl372.c +18 -5

index e3ecbaee61f7..bc53af809d5d 100644
@@ -998,17 +998,30 @@ static ssize_t adxl372_get_fifo_watermark(struct device *dev,
 	return sprintf(buf, "%d\n", st->watermark);
 }
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(ADXL372_FIFO_SIZE));
+static ssize_t hwfifo_watermark_min_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "1");
+}
+
+static ssize_t hwfifo_watermark_max_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", __stringify(ADXL372_FIFO_SIZE));
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_min, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
 static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
 		       adxl372_get_fifo_watermark, NULL, 0);
 static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
 		       adxl372_get_fifo_enabled, NULL, 0);
 
 static const struct attribute *adxl372_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,

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

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/accel/bmc150-accel-core.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

drivers/iio/accel/bmc150-accel-core.c +18 -5

index 57e8a8350cd1..92f8b139acce 100644
@@ -925,17 +925,30 @@ static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
 	{ }
 };
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(BMC150_ACCEL_FIFO_LENGTH));
+static ssize_t hwfifo_watermark_min_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "1");
+}
+
+static ssize_t hwfifo_watermark_max_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", __stringify(BMC150_ACCEL_FIFO_LENGTH));
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_min, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
 static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
 		       bmc150_accel_get_fifo_state, NULL, 0);
 static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
 		       bmc150_accel_get_fifo_watermark, NULL, 0);
 
 static const struct attribute *bmc150_accel_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,

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

Fixes 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")

Quoted 13 lines · Matti Vaittinen

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.

Quoted 2 lines

Signed-off-by Matti Vaittinen <[email protected]>

Fixes 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr")

Tested-by Claudiu Beznea <[email protected]>

on SAMA5D2

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

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.

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

drivers/iio/industrialio-buffer.c +0 -0

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

Quoted 11 lines · Matti Vaittinen

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.

Quoted 2 lines

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/industrialio-buffer.c static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, · line 1602

Quoted 5 lines

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

drivers/iio/industrialio-buffer.c +1 -0

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

Quoted 4 lines

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

...

drivers/iio/industrialio-buffer.c static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, · line 1671

Quoted 2 lines

drivers/iio/industrialio-buffer.c +2 -0

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

Not sure why we have additional parentheses...

drivers/iio/industrialio-buffer.c static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, · line 1673

Quoted 2 lines

drivers/iio/industrialio-buffer.c +2 -0

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

Hi Andy,

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

Quoted 20 lines · Andy Shevchenko

Quoted 13 lines · Matti Vaittinen

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.

...

Quoted 2 lines

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

Quoted 1 line

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

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:

Quoted 14 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 11 lines

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?

Quoted 1 line · Andy Shevchenko

On Mon, Oct 03, 2022 at 11:13:53AM +0300, Matti Vaittinen wrote:

Quoted 4 lines

drivers/iio/industrialio-buffer.c static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, · line 1671

Quoted 2 lines

drivers/iio/industrialio-buffer.c +2 -0

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

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

Quoted 11 lines · Matti Vaittinen

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.

Quoted 4 lines

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

Signed-off-by Matti Vaittinen <[email protected]>

Tested-by Claudiu Beznea <[email protected]>

on SAMA5D2

Hi Claudiu,

Quoted 19 lines · On 10/6/22 11:35, [email protected] wrote:

Quoted 7 lines · Matti Vaittinen

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

Quoted 8 lines

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

Tested-by Claudiu Beznea <[email protected]>

Quoted 2 lines

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

Quoted 22 lines · On Thu, 6 Oct 2022 15:53:52 +0300 Matti Vaittinen <[email protected]>…

Hi Claudiu,

Quoted 19 lines · On 10/6/22 11:35, [email protected] wrote:

Quoted 7 lines · Matti Vaittinen

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

Quoted 8 lines

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

Tested-by Claudiu Beznea <[email protected]>

Quoted 7 lines

Quoted 2 lines

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,

Quoted 39 lines · Jonathan Cameron

Quoted 22 lines · On Thu, 6 Oct 2022 15:53:52 +0300 Matti Vaittinen <[email protected]>…

Hi Claudiu,

Quoted 19 lines · On 10/6/22 11:35, [email protected] wrote:

Quoted 7 lines · Matti Vaittinen

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

Quoted 8 lines

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

Tested-by Claudiu Beznea <[email protected]>

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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