On Sat, 30 Oct 2021 17:12:02 +0200
Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> 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.
Maybe. Originally thinking was that some devices would schedule work to
complete the read so it might not correspond to IRQ_HANDLED.
I have no idea if there are any drivers still doing that though.
>
> 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.
Indeed - the goal is good, but might take some doing to get there!
Jonathan
