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

[Jonathan Cameron]

On Mon, 19 Sep 2022 16:32:14 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

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

Thinking a little more on this - all / (most?) of the users pass a null terminated
array of struct device_attribute * to *iio_triggered_buffer_setup_ext()

That's then assigned to buffer->attrs. 
We could add an additional pointer to the struct iio_buffer to take
a null terminated array of struct iio_dev_attr *
and change the signature of that function to take one of those, thus
preventing us using iio_const_attr structures for this.

Then we can wrap those just fine in the code you highlighted and assign the
result into buffer->attrs.

We'd need to precede that change with fixes that just switch the
iio_const_attr uses over to iio_dev_attr but changing this would ensure no
accidental reintroductions of the problem in future drivers (typically
as a result of someone forward porting a driver that is out of tree).

I think this combination of fix then prevent future problems is what
I would prefer.

Jonathan



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