Re: [PATCH 1/2] iio: mma8452: Fix trigger reference couting

From: Jonathan Cameron <jic23@kernel.org>
To: Lars-Peter Clausen <lars@metafoo.de>
Cc: Martin Fuzzey <mfuzzey@parkeon.com>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-iio@vger.kernel.org
Subject: Re: [PATCH 1/2] iio: mma8452: Fix trigger reference couting
Date: Sat, 30 Oct 2021 16:03:57 +0100	[thread overview]
Message-ID: <20211030160357.6191e2a8@jic23-huawei> (raw)
In-Reply-To: <3bf78fdf-c6df-dd77-a1f1-61800c0ebe37@metafoo.de>

On Thu, 28 Oct 2021 21:52:46 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 10/28/21 4:07 PM, Jonathan Cameron wrote:
> > On Sun, 24 Oct 2021 11:26:59 +0200
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >  
> >> The mma8452 driver directly assigns a trigger to the struct iio_dev. The
> >> IIO core when done using this trigger will call `iio_trigger_put()` to drop
> >> the reference count by 1.
> >>
> >> Without the matching `iio_trigger_get()` in the driver the reference count
> >> can reach 0 too early, the trigger gets freed while still in use and a
> >> use-after-free occurs.
> >>
> >> Fix this by getting a reference to the trigger before assigning it to the
> >> IIO device.
> >>
> >> Fixes: ae6d9ce05691 ("iio: mma8452: Add support for interrupt driven triggers.")
> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>  
> > Gah. I thought we'd gotten all these years ago. I guess this one slipped through
> > the net.  
> Btw. we already have iio_trigger_set_immutable(), which handles the 
> reference counting. I was think of adding a iio(_device)_trigger_set() 
> that does the same except not setting the trig_readonly flag. And then 
> eventually move the trigger to iio_dev_opaque. Any concerns with this?

No concerns, seems like as sensible change given how things are evolving.
Obviously some other stuff that would need changing before we can
actually move trig.

One early step would be to modify iio_trigger_notify_done() to take
a struct iio_dev rather than a struct iio_trigger.  A job for a coccinelle
script I think!  That function name might need a rethink along with the
parameter change.

Hmm. Looks like we have a few drivers passing indio_dev->trig to iio_trigger_poll
as well which is a little odd.  mma8452 is one of them and it's not using
an immutable trigger or validate_trigger() so unexpected results if anyone
changes the trigger...  Possibly not fatal as the interrupt will probably
not occur but not correct either...

Jonathan