[RFT PATCH v2 2/2] iio: Fix unsafe buffer attributes

From: Matti Vaittinen
Date: Sat Oct 01 2022 - 03:44:44 EST

Next message: Dmitry Baryshkov: "Re: [PATCH 2/2] pinctrl: qcom: Add QDU1000/QRU1000 pinctrl driver"
Previous message: Matti Vaittinen: "[RFT PATCH v2 1/2] iio: Add IIO_STATIC_CONST_DEVICE_ATTR"
In reply to: Matti Vaittinen: "[RFT PATCH v2 1/2] iio: Add IIO_STATIC_CONST_DEVICE_ATTR"
Next in thread: Jonathan Cameron: "Re: [RFT PATCH v2 2/2] iio: Fix unsafe buffer attributes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

---

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 | 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/triggered_buffer.h | 6 +++---
13 files changed, 58 insertions(+), 54 deletions(-)

diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index 47feb375b70b..0922ac0fad9e 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -1185,19 +1185,19 @@ 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));
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
+ __stringify(ADXL367_FIFO_MAX_WATERMARK));
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.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 e3ecbaee61f7..c4193286eb05 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -998,19 +998,19 @@ 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));
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
+ __stringify(ADXL372_FIFO_SIZE));
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.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 57e8a8350cd1..110591804b4c 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -925,19 +925,19 @@ 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));
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
+ __stringify(BMC150_ACCEL_FIFO_LENGTH));
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.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 279430c1d88c..f994366b0778 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1846,8 +1846,8 @@ static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
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);
+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) " "
@@ -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_const_attr_hwfifo_watermark_min.dev_attr.attr,
- &iio_const_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: signature.asc
Description: PGP signature

Next message: Dmitry Baryshkov: "Re: [PATCH 2/2] pinctrl: qcom: Add QDU1000/QRU1000 pinctrl driver"
Previous message: Matti Vaittinen: "[RFT PATCH v2 1/2] iio: Add IIO_STATIC_CONST_DEVICE_ATTR"
In reply to: Matti Vaittinen: "[RFT PATCH v2 1/2] iio: Add IIO_STATIC_CONST_DEVICE_ATTR"
Next in thread: Jonathan Cameron: "Re: [RFT PATCH v2 2/2] iio: Fix unsafe buffer attributes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]