iio: mma8452: Fix trigger reference couting

-----Original Message-----
From: Lars-Peter Clausen <[email protected]>
Sent: Sunday, October 24, 2021 11:27 AM
To: Jonathan Cameron <[email protected]>
Cc: Martin Fuzzey <[email protected]>; Peter Meerwald-Stadler <[email protected]>; [email protected]; Lars-Peter Clausen <[email protected]>
Subject: [PATCH 2/2] iio: trigger: Fix reference counting

[External]

In viio_trigger_alloc() device_initialize() is used to set the initial reference count of the trigger to 1. Then another get_device() is called on
trigger. This sets the reference count to 2 before the trigger is returned.

iio_trigger_free(), which is the matching API to viio_trigger_alloc(), calls put_device() which decreases the reference count by 1. But the second
reference count acquired in viio_trigger_alloc() is never dropped.

As a result the iio_trigger_release() function is never called and the memory associated with the trigger is never freed.

Since there is no reason for the trigger to start its lifetime with two reference counts just remove the extra get_device() in viio_trigger_alloc().

Fixes 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the

core for triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

---
I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the point when this was introduced and I believe it was incorrect even back
then.

But we also had a few drivers that directly assigned the indio_dev->trig without getting an extra reference. So these two bugs, one in the core, one
in the drivers sort of even out. Except that iio_trigger_get() also gets a reference to the drivers module and iio_trigger_put() releases it again. So
with the missing iio_trigger_get() there is still the problem that, even though the device references balance out, there is a module reference count
imbalance.

---
 drivers/iio/industrialio-trigger.c | 1 -
 1 file changed, 1 deletion(-)

drivers/iio/industrialio- renamed from drivers/iio/industrialio-trigger.c +0 -0

trigger.c
index b23caa2f2aa1..93990ff1dfe3 100644

drivers/iio/industrialio-trigger.c +0 -1

@@ -556,7 +556,6 @@ struct iio_trigger *viio_trigger_alloc(struct
 device *parent,
 		irq_modify_status(trig->subirq_base + i,
 				  IRQ_NOREQUEST | IRQ_NOAUTOEN,
 IRQ_NOPROBE);
 	}
-	get_device(&trig->dev);
 

return trig;

--
2.20.1

In viio_trigger_alloc() device_initialize() is used to set the initial reference count of the trigger to 1. Then another get_device() is called on trigger. This sets the reference count to 2 before the trigger is returned.

iio_trigger_free(), which is the matching API to viio_trigger_alloc(), calls put_device() which decreases the reference count by 1. But the second reference count acquired in viio_trigger_alloc() is never dropped.

As a result the iio_trigger_release() function is never called and the memory associated with the trigger is never freed.

Since there is no reason for the trigger to start its lifetime with two reference counts just remove the extra get_device() in viio_trigger_alloc().

Fixes 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

---
I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the point when this was introduced and I believe it was incorrect even back then.

But we also had a few drivers that directly assigned the indio_dev->trig without getting an extra reference. So these two bugs, one in the core, one in the drivers sort of even out. Except that iio_trigger_get() also gets a reference to the drivers module and iio_trigger_put() releases it again. So with the missing iio_trigger_get() there is still the problem that, even though the device references balance out, there is a module reference count imbalance.

---
 drivers/iio/industrialio-trigger.c | 1 -
 1 file changed, 1 deletion(-)

drivers/iio/industrialio-trigger.c +0 -1

index b23caa2f2aa1..93990ff1dfe3 100644
@@ -556,7 +556,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 		irq_modify_status(trig->subirq_base + i,
 				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
 	}
-	get_device(&trig->dev);
 
 	return trig;
 

Quoted 11 lines · On Sun, 24 Oct 2021 11:27:00 +0200 Lars-Peter Clausen <[email protected]> wrote:

In viio_trigger_alloc() device_initialize() is used to set the initial reference count of the trigger to 1. Then another get_device() is called on trigger. This sets the reference count to 2 before the trigger is returned.

iio_trigger_free(), which is the matching API to viio_trigger_alloc(), calls put_device() which decreases the reference count by 1. But the second reference count acquired in viio_trigger_alloc() is never dropped.

As a result the iio_trigger_release() function is never called and the memory associated with the trigger is never freed.

Quoted 6 lines

Since there is no reason for the trigger to start its lifetime with two reference counts just remove the extra get_device() in viio_trigger_alloc().

Fixes 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

I fully agree the current code is wrong, but we really should be using device_put() in the error path after device_initialize() has been called.

There are multiple places where we currently do this wrong in IIO but this particular one looks like a local fix would be safe.
Worth doing that in the same patch at this one given it's all about reference counting logic being wrong? If not, we can do it as a separate follow up patch.

Jonathan

Quoted 28 lines

---
I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the point when this was introduced and I believe it was incorrect even back then.

But we also had a few drivers that directly assigned the indio_dev->trig without getting an extra reference. So these two bugs, one in the core, one in the drivers sort of even out. Except that iio_trigger_get() also gets a reference to the drivers module and iio_trigger_put() releases it again. So with the missing iio_trigger_get() there is still the problem that, even though the device references balance out, there is a module reference count imbalance.

---
  drivers/iio/industrialio-trigger.c | 1 -
  1 file changed, 1 deletion(-)

drivers/iio/industrialio-trigger.c +0 -1

index b23caa2f2aa1..93990ff1dfe3 100644
@@ -556,7 +556,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
  		irq_modify_status(trig->subirq_base + i,
  				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
  	}
-	get_device(&trig->dev);
 
  	return trig;
 

Quoted 23 lines · Jonathan Cameron

Quoted 11 lines · On Sun, 24 Oct 2021 11:27:00 +0200 Lars-Peter Clausen <[email protected]> wrote:

In viio_trigger_alloc() device_initialize() is used to set the initial reference count of the trigger to 1. Then another get_device() is called on trigger. This sets the reference count to 2 before the trigger is returned.

iio_trigger_free(), which is the matching API to viio_trigger_alloc(), calls put_device() which decreases the reference count by 1. But the second reference count acquired in viio_trigger_alloc() is never dropped.

As a result the iio_trigger_release() function is never called and the memory associated with the trigger is never freed.

Quoted 6 lines

Since there is no reason for the trigger to start its lifetime with two reference counts just remove the extra get_device() in viio_trigger_alloc().

Fixes 5f9c035cae18 ("staging:iio:triggers. Add a reference get to the core for triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

I fully agree the current code is wrong, but we really should be using device_put() in the error path after device_initialize() has been called.

Quoted 4 lines

There are multiple places where we currently do this wrong in IIO but this particular one looks like a local fix would be safe.
Worth doing that in the same patch at this one given it's all about reference counting logic being wrong? If not, we can do it as a separate follow up patch.

I already have that patch. Just waiting for this to be applied since it has a dependency.

Quoted 32 lines

Jonathan

Quoted 28 lines

---
I'm a bit unsure about the fixes tag. I've looked at the IIO tree at the point when this was introduced and I believe it was incorrect even back then.

But we also had a few drivers that directly assigned the indio_dev->trig without getting an extra reference. So these two bugs, one in the core, one in the drivers sort of even out. Except that iio_trigger_get() also gets a reference to the drivers module and iio_trigger_put() releases it again. So with the missing iio_trigger_get() there is still the problem that, even though the device references balance out, there is a module reference count imbalance.

---
  drivers/iio/industrialio-trigger.c | 1 -
  1 file changed, 1 deletion(-)

drivers/iio/industrialio-trigger.c +0 -1

index b23caa2f2aa1..93990ff1dfe3 100644
@@ -556,7 +556,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
  		irq_modify_status(trig->subirq_base + i,
  				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
  	}
-	get_device(&trig->dev);
 
  	return trig;
 

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 <[email protected]>

---
 drivers/iio/accel/mma8452.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/accel/mma8452.c +1 -1

index 715b8138fb71..09c7f10fefb6 100644
@@ -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;
 }

Quoted 8 lines · On Sun, 24 Oct 2021 11:26:59 +0200 Lars-Peter Clausen <[email protected]> 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.

Quoted 5 lines

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 <[email protected]>

Gah. I thought we'd gotten all these years ago. I guess this one slipped through the net.

Quoted 14 lines · Jonathan Cameron

Quoted 11 lines · On Sun, 24 Oct 2021 11:26:59 +0200 Lars-Peter Clausen <[email protected]> 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.

Quoted 4 lines

Quoted 2 lines

Fixes ae6d9ce05691 ("iio: mma8452: Add support for interrupt driven triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

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?

Quoted 15 lines · On Thu, 28 Oct 2021 21:52:46 +0200 Lars-Peter Clausen <[email protected]> wrote:

Quoted 14 lines · Jonathan Cameron

Quoted 11 lines · On Sun, 24 Oct 2021 11:26:59 +0200 Lars-Peter Clausen <[email protected]> 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.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Fixes ae6d9ce05691 ("iio: mma8452: Add support for interrupt driven triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

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...

Quoted 30 lines · Jonathan Cameron

Quoted 15 lines · On Thu, 28 Oct 2021 21:52:46 +0200 Lars-Peter Clausen <[email protected]> wrote:

Quoted 14 lines · Jonathan Cameron

Quoted 11 lines · On Sun, 24 Oct 2021 11:26:59 +0200 Lars-Peter Clausen <[email protected]> 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.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Fixes ae6d9ce05691 ("iio: mma8452: Add support for interrupt driven triggers.")

Signed-off-by Lars-Peter Clausen <[email protected]>

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 5 lines

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.