On 10/30/21 5:03 PM, Jonathan Cameron wrote:
On Thu, 28 Oct 2021 21:52:46 +0200
Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
On 10/28/21 4:07 PM, Jonathan Cameron wrote:
On Sun, 24 Oct 2021 11:26:59 +0200
Lars-Peter Clausen <lars@xxxxxxxxxx> 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@xxxxxxxxxx>
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.
