Re: [PATCH 1/2] iio: mma8452: Fix trigger reference couting - Lars-Peter Clausen

From: Lars-Peter Clausen <lars@metafoo.de>
To: Jonathan Cameron <jic23@kernel.org>
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 17:12:02 +0200	[thread overview]
Message-ID: <86c1aa49-edc4-c02c-7ab7-4d075819a847@metafoo.de> (raw)
In-Reply-To: <20211030160357.6191e2a8@jic23-huawei>

On 10/30/21 5:03 PM, Jonathan Cameron wrote:
> 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.

That was my first idea, but then I was like why do we even have to call 
notify_done()? Can't we automate this, given all the bugs we had around 
this over the years.

Sill work in progress: 
https://github.com/larsclausen/linux/commit/d6ed694c9e512e1f7f3b40ad06b153feca8d7bb1 
but I think this will work.

>
> 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...
Yep, that's on my radar too. And one of the reasons to move the trigger 
to the opaque structure so this type of error can not happen.