Re: [PATCH] iio: trigger: fix use-after-free in current_trigger_store() on concurrent writers

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 26 Oct 2026 20:02:46 +1000
Yu Zhang <yuz08559@gmail.com> wrote:

> current_trigger_store() holds iio_dev_opaque->mlock only around the
> INDIO_BUFFER_TRIGGERED / trig_readonly checks and then drops it. The rest
> runs unlocked: it reads the current trigger into oldtrig, acquires the new
> one by name, stores it into indio_dev->trig, and drops the old trigger's
> reference with iio_trigger_put().
> 
> sysfs does not serialize writes to the same attribute issued through
> different open file descriptors (each open has its own kernfs_open_file),
> so two threads can run current_trigger_store() on the same
> .../trigger/current_trigger concurrently. Both read the same oldtrig, both
> overwrite indio_dev->trig, and both call iio_trigger_put(oldtrig). The
> extra put drops the trigger's refcount to zero while it is still in use,
> frees it, and a concurrent writer then dereferences the freed trigger.
> 
> With two sets of writers hammering current_trigger between two software
> triggers on an iio_dummy device, KASAN reports:
> 
>   refcount_t: addition on 0; use-after-free.
>   WARNING: CPU: 3 PID: 3467 at refcount_warn_saturate+0xbb/0xe0
>    kobject_get+0xf6/0x100
>    current_trigger_store+0x15c/0x4b0
>   ...
>   BUG: KASAN: slab-use-after-free in current_trigger_store+0x168/0x4b0
>   Freed by task 3468:
>    kobject_put+0x125/0x220
>    current_trigger_store+0x2cc/0x4b0
>   Allocated by task 3454:
>    __iio_trigger_alloc+0xc7/0x100
>   ...
>   Oops: general protection fault
>    __module_get+0x27/0xd0
>    current_trigger_store+0x192/0x4b0
> 
> Hold mlock across the whole operation so that reading oldtrig, swapping
> indio_dev->trig and dropping the reference are atomic with respect to
> other writers (and to buffer enable, which the -EBUSY check guards).
> Acquiring the new trigger with iio_trigger_acquire_by_name() under mlock
> is safe: it only takes iio_trigger_list_lock, and the buffer path already
> calls iio_trigger_{attach,detach}_poll_func() with mlock held.

Sashiko correctly raises that the change means more directly
mixing goto and cleanup.h magic than before.  To make this change
we'll need to unwind that and just do mutex_lock / mutex_unlock or
figure out another scheme using a helper.
https://sashiko.dev/#/patchset/20261026100246.68005-1-yuz08559%40gmail.com

Other than that, the fix feels correct but I would like some
eyes on this one.

Jonathan

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>
> ---
>  drivers/iio/industrialio-trigger.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 76926fe..9241cd6 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -441,17 +441,18 @@ static ssize_t current_trigger_store(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> -	struct iio_trigger *oldtrig = indio_dev->trig;
> +	struct iio_trigger *oldtrig;
>  	struct iio_trigger *trig;
>  	int ret;
>  
> -	scoped_guard(mutex, &iio_dev_opaque->mlock) {
> -		if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED)
> -			return -EBUSY;
> -		if (iio_dev_opaque->trig_readonly)
> -			return -EPERM;
> -	}
> +	guard(mutex)(&iio_dev_opaque->mlock);
> +
> +	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED)
> +		return -EBUSY;
> +	if (iio_dev_opaque->trig_readonly)
> +		return -EPERM;
>  
> +	oldtrig = indio_dev->trig;
>  	trig = iio_trigger_acquire_by_name(buf);
>  	if (oldtrig == trig) {
>  		ret = len;