Re: [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers

[Jonathan Cameron]

On Mon, 19 Sep 2022 08:52:38 +0000
"Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:

> On 9/9/22 11:12, Vaittinen, Matti wrote:
> > 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 :)  
> 
> I would like to add at least the at91-sama5d2_adc (conditonally 
> registers the IIO_CONST_ATTR for triggered-buffer) to the list of 
> devices that could be potentially tested. I hope some of these devices 
> had a user who could either make us worried and verify my assumption - 
> or make me ashamed but rest of us relieved :) Eg - I second my request 
> for testing this - and add potential owners of at91-sama5d2_adc to the list.
> 
> > 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.
> >  
> 
> and at91-sama5d2_adc
> 
> //snip
> 
> >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.  
> 
> Yours,
> 	-- Matti
Hi Matti,

+CC Alexandru on a current email address.

I saw this whilst travelling and completely forgot about when
I was back to normal - so great you sent a follow up!

Anyhow, your reasoning seems correct and it would be easy enough
to add such a case to iio/dummy/iio_simple_dummy_buffer.c and
provide a clear test for the problem.

As to solutions. The quickest is probably to switch these const attrs
over to a non const form and add a comment to the header to say they are
unsuitable for use with buffers.

An alternative would be to make it 'safe' by making the data layouts
match up.

struct iio_attr {
	struct device_attribute dev_attr;
	union {
		u64 address;
		const char *string;
	};
	struct list_head l;
	struct iio_chan_spec const *c;
	struct iio_buffer *buffer;
};

#define iio_dev_attr iio_attr
#define iio_const_attr iio_attr

Looking at this raises another potential problem.
Where is the address copied over for attributes using IIO_DEVICE_ATTR()?
Maybe I'm just missing it somewhere.  Grepping suggests we've been
lucky and there are no users of that field in buffer attributes.

Detecting the problem you found is going to be inherently tricky - though maybe
could rely on the naming of the attributes passed in (iio_const...)
and some scripting magic. 

Longer term, it's this sort of thing that motivates protections / runnable
CI self tests with, for example, the roadtest framework that I'm hoping
will be available upstream soonish!

Would you like to send patches given you identified the problem?

If not I'm happy to fix these up. My grepping identified the same 3 cases
you found.

Jonathan