| Message ID | 20211024092700.6844-1-lars@metafoo.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] iio: mma8452: Fix trigger reference couting | expand |
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. Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > --- > drivers/iio/accel/mma8452.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c > index 715b8138fb71..09c7f10fefb6 100644 > --- a/drivers/iio/accel/mma8452.c > +++ b/drivers/iio/accel/mma8452.c > @@ -1470,7 +1470,7 @@ static int mma8452_trigger_setup(struct iio_dev *indio_dev) > if (ret) > return ret; > > - indio_dev->trig = trig; > + indio_dev->trig = iio_trigger_get(trig); > > return 0; > }
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?
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
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.
On Sat, 30 Oct 2021 17:12:02 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > 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. 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
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c index 715b8138fb71..09c7f10fefb6 100644 --- a/drivers/iio/accel/mma8452.c +++ b/drivers/iio/accel/mma8452.c @@ -1470,7 +1470,7 @@ static int mma8452_trigger_setup(struct iio_dev *indio_dev) if (ret) return ret; - indio_dev->trig = trig; + indio_dev->trig = iio_trigger_get(trig); return 0; }
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> --- drivers/iio/accel/mma8452.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)