[PATCH] iio: gyro: mpu3050: add missing trigger cleanup on remove

[PATCH] iio: gyro: mpu3050: add missing trigger cleanup on remove

[Joshua Crofts]

The mpu3050_trigger_probe() function registers a trigger but
mpu3050_common_remove() and error paths in mpu3050_common_probe() forget
to clean it up, causing a use-after-free bug.

Add the missing iio_trigger_unregister() function.

Reported-by: Dingisoul <dingiso.kernel@gmail.com>
Closes: https://lore.kernel.org/all/20261010203601.1686148-1-dingiso.kernel@gmail.com/
Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
Cc: stable@vger.kernel.org
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/gyro/mpu3050-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index 07c0f9011c63..096fd0dc608e 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -1260,6 +1260,8 @@ int mpu3050_common_probe(struct device *dev,
 	pm_runtime_get_sync(dev);
 	pm_runtime_put_noidle(dev);
 	pm_runtime_disable(dev);
+	if (mpu3050->trig)
+		iio_trigger_unregister(mpu3050->trig);
 	if (irq)
 		free_irq(mpu3050->irq, mpu3050->trig);
 	iio_triggered_buffer_cleanup(indio_dev);
@@ -1278,6 +1280,8 @@ void mpu3050_common_remove(struct device *dev)
 	pm_runtime_get_sync(dev);
 	pm_runtime_put_noidle(dev);
 	pm_runtime_disable(dev);
+	if (mpu3050->trig)
+		iio_trigger_unregister(mpu3050->trig);
 	if (mpu3050->irq)
 		free_irq(mpu3050->irq, mpu3050->trig);
 	iio_triggered_buffer_cleanup(indio_dev);

---
base-commit: a3b3580713f3ac5a32dc2874ee546828977a1d68
change-id: 20261011-mpu3050-fix-145a77d5229b

Best regards,
-- 
Joshua Crofts

Re: [PATCH] iio: gyro: mpu3050: add missing trigger cleanup on remove

[Joshua Crofts]

On Sun, 11 Oct 2026 17:53:01 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>  drivers/iio/gyro/mpu3050-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Sashiko review came back and the issue doesn't seem so
straight-forward. I'll probably introduce a local pointer
and then assign it to the mpu3050 data struct.

Attaching Sashiko review below.

> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index 07c0f9011c63..096fd0dc608e 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -1260,6 +1260,8 @@ int mpu3050_common_probe(struct device *dev,
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> +	if (mpu3050->trig)
> +		iio_trigger_unregister(mpu3050->trig);


Does this unconditionally unregister a trigger that may not have been
successfully registered?

If mpu3050_trigger_probe() fails (for example, if request_threaded_irq()
fails), mpu3050->trig is allocated via devm_iio_trigger_alloc() but it will
never have successfully completed iio_trigger_register().

mpu3050_common_probe() catches that error, logs "failed to register trigger",
and continues. If a subsequent step like iio_device_register() fails, this
code will call iio_trigger_unregister() on the uninitialized trigger.

Because the trigger was never registered via iio_trigger_register(),
iio_trigger_unregister() will call device_del() on a device that was never
added, and ida_free() on an unallocated ID, which can lead to a crash.

>  	if (irq)
>  		free_irq(mpu3050->irq, mpu3050->trig);
>  	iio_triggered_buffer_cleanup(indio_dev);
> @@ -1278,6 +1280,8 @@ void mpu3050_common_remove(struct device *dev)
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> +	if (mpu3050->trig)
> +		iio_trigger_unregister(mpu3050->trig);

Can this cause a kernel crash during normal driver teardown if the trigger
probe originally failed?

Similar to the probe error path above, if mpu3050_trigger_probe() failed
during initialization, mpu3050->trig is left allocated but unregistered.

Calling iio_trigger_unregister() here on that uninitialized trigger will
cause the same teardown issues during driver removal.

-- 
Kind regards,
Joshua Crofts

Re: [PATCH] iio: gyro: mpu3050: add missing trigger cleanup on remove

[Nuno Sá]

On Sun, Oct 11, 2026 at 07:12:40PM +0200, Joshua Crofts wrote:
> On Sun, 11 Oct 2026 17:53:01 +0200
> Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >  drivers/iio/gyro/mpu3050-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> 
> Sashiko review came back and the issue doesn't seem so
> straight-forward. I'll probably introduce a local pointer
> and then assign it to the mpu3050 data struct.

Hmm the probe seems a mess of mixed devm with manual cleanup. Ideally we
would move all of it managed resources (and that would fix the issue).
Other than that, seems to me we have two things in play. First:

if (irq) {
	ret = mpu3050_trigger_probe(indio_dev, irq);
	if (ret)
		dev_err(dev, "failed to register trigger\n");
}

Do we really want to ignore the error? I would say no... 
So, if we change the above to not ignore errors, we can assume that, on
success, our trigger was registered. Which means that on the if (irq) at
the errir patch we could unregister the trigger. Not that pretty but 
consistent with what we have :).

If someone has the HW, moving this into devm would probably be the
better approach (but even in that case, we might want the simple fix
first for backporting).

- Nuno Sá

> 
> Attaching Sashiko review below.
> 
> > diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> > index 07c0f9011c63..096fd0dc608e 100644
> > --- a/drivers/iio/gyro/mpu3050-core.c
> > +++ b/drivers/iio/gyro/mpu3050-core.c
> > @@ -1260,6 +1260,8 @@ int mpu3050_common_probe(struct device *dev,
> >  	pm_runtime_get_sync(dev);
> >  	pm_runtime_put_noidle(dev);
> >  	pm_runtime_disable(dev);
> > +	if (mpu3050->trig)
> > +		iio_trigger_unregister(mpu3050->trig);
> 
> 
> Does this unconditionally unregister a trigger that may not have been
> successfully registered?
> 
> If mpu3050_trigger_probe() fails (for example, if request_threaded_irq()
> fails), mpu3050->trig is allocated via devm_iio_trigger_alloc() but it will
> never have successfully completed iio_trigger_register().
> 
> mpu3050_common_probe() catches that error, logs "failed to register trigger",
> and continues. If a subsequent step like iio_device_register() fails, this
> code will call iio_trigger_unregister() on the uninitialized trigger.
> 
> Because the trigger was never registered via iio_trigger_register(),
> iio_trigger_unregister() will call device_del() on a device that was never
> added, and ida_free() on an unallocated ID, which can lead to a crash.
> 
> >  	if (irq)
> >  		free_irq(mpu3050->irq, mpu3050->trig);
> >  	iio_triggered_buffer_cleanup(indio_dev);
> > @@ -1278,6 +1280,8 @@ void mpu3050_common_remove(struct device *dev)
> >  	pm_runtime_get_sync(dev);
> >  	pm_runtime_put_noidle(dev);
> >  	pm_runtime_disable(dev);
> > +	if (mpu3050->trig)
> > +		iio_trigger_unregister(mpu3050->trig);
> 
> Can this cause a kernel crash during normal driver teardown if the trigger
> probe originally failed?
> 
> Similar to the probe error path above, if mpu3050_trigger_probe() failed
> during initialization, mpu3050->trig is left allocated but unregistered.
> 
> Calling iio_trigger_unregister() here on that uninitialized trigger will
> cause the same teardown issues during driver removal.
> 
> -- 
> Kind regards,
> Joshua Crofts