by Andy Shevchenko

[permalink] [raw]

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

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

2022-10-03 09:12:18

by 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! ~~

2022-10-03 09:30:57

by Matti Vaittinen

[permalink] [raw]

Subject: [PATCH v3 02/10] iio: adxl372: Fix unsafe buffer attributes

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

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index e3ecbaee61f7..bc53af809d5d 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -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,
--
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 =]

Attachments:

(No filename) (2.77 kB)
signature.asc (499.00 B)
Download all attachments

2022-10-03 09:56:18

On Mon, 10 Oct 2022 12:36:54 +0300
Matti Vaittinen <[email protected]> wrote:

> On 10/9/22 20:38, Jonathan Cameron wrote:
> > On Thu, 6 Oct 2022 15:53:52 +0300
> > Matti Vaittinen <[email protected]> wrote:
> >
> >> Hi Claudiu,
> >>
> >> On 10/6/22 11:35, [email protected] 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 <[email protected]>
> >>>
> >>> Tested-by: Claudiu Beznea <[email protected]>
> >>>
> >>> 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
>