linux-kernel - Re: [PATCH v6 14/24] iio: buffer: wrap all buffer attributes into iio_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Sep 2022 08:12:37 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
CC:     "jic23@...nel.org" <jic23@...nel.org>,
        "nuno.sa@...log.com" <nuno.sa@...log.com>,
        "dragos.bogdan@...log.com" <dragos.bogdan@...log.com>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        Stefan Popa <stefan.popa@...log.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Michael Hennerich <michael.hennerich@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 14/24] iio: buffer: wrap all buffer attributes into
 iio_dev_attr

Hi dee Ho peeps!

Disclaimer - I have no HW to test this using real in-tree drivers. If 
someone has a device with a variant of bmc150 or adxl372 or  - it'd be 
nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min 
works with the v6.0-rc4. Maybe I am misreading code and have my own 
issues - in which case I apologize already now and go to the corner 
while being deeply ashamed :)

On 2/15/21 12:40, Alexandru Ardelean wrote:
> This change wraps all buffer attributes into iio_dev_attr objects, and
> assigns a reference to the IIO buffer they belong to.
> 
> With the addition of multiple IIO buffers per one IIO device, we need a way
> to know which IIO buffer is being enabled/disabled/controlled.
> 
> We know that all buffer attributes are device_attributes. 

I think this assumption is slightly unsafe. I see few drivers adding 
IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 
add the hwfifo_watermark_min and hwfifo_watermark_max.

Long story short (or the other way around?)

I was developing a driver for ROHM/Kionix KX022A accelerometer. I did a 
random pick and chose the bmc150-core as a reference how others have 
implemented the accel drivers. During the testing I noticed that using 
IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere 
it shouldn't... Oops.

Reading the code allows me to assume the problem is wrapping the 
attributes to IIO_DEV_ATTRs.

static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer,
+					      struct attribute *attr)
+{
+	struct device_attribute *dattr = to_dev_attr(attr);
+	struct iio_dev_attr *iio_attr;
+
+	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
+	if (!iio_attr)
+		return NULL;
+
+	iio_attr->buffer = buffer;
+	memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));

This copy does assume all attributes are device_attrs, and does not take 
into account that IIO_CONST_ATTRS have the string stored in a struct 
iio_const_attr which is containing the dev_attr. Eg, copying in the 
iio_buffer_wrap_attr() does not copy the string - and later invoking the 
'show' callback goes reading something else than the mentioned string 
because the pointer is not copied.

I believe problem has been there for a while now - introduced by this:
15097c7a1adc ("iio: buffer: wrap all buffer attributes into 
iio_dev_attr") at Feb 2021.

I don't have a fix for you as I am not sure what would be the correct 
fix. We _could_ go through all the IIO drivers and ensure none used 
IIO_CONST_ATTRs for triggered buffers. This would IMHO be just a 
workaround to the real problem - which is assumption that all attributes 
in the attribute group are dev_attrs. Such silent assumptions are 
fragile as we see :)

If we opt treating all attributes in the group as device_attrs also in 
the future, then (in my not always so humble opinion) the APIs should 
force this - but I'm not sure how to do that.

Best regards
	-- Matti

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

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