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

[Jonathan Cameron <jic23@kernel.org>]

On Mon,  2 Nov 2026 13:45:32 +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.
> 
> The trigger swap itself keeps its goto based cleanup of the newly
> acquired trigger, so move it into a helper and take mlock with guard()
> in current_trigger_store() around the mode checks and the helper call,
> rather than mixing guard() and goto in one function.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>
> ---

A process thing.  The change log that you have at end of
the file goes here.

Also don't send a new version in reply to a previous one.
Sometimes it confuses tools, but more relevantly it
ensures that your thread is way way back in a kernel
reviewers email so by the time the reach if if they ever
do they are half asleep!

Otherwise nothing to add on the actual patch to what Andy
already sent.

>  drivers/iio/industrialio-trigger.c | 60 +++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 76926fe..cfc6af0 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -421,37 +421,14 @@ static ssize_t current_trigger_show(struct device *dev,
>  	return 0;
>  }
>  
> -/**
> - * current_trigger_store() - trigger consumer sysfs set current trigger
> - * @dev:	device associated with an industrial I/O device
> - * @attr:	device attribute that is being processed
> - * @buf:	string buffer that holds the name of the trigger
> - * @len:	length of the trigger name held by buf
> - *
> - * For trigger consumers the current_trigger interface allows the trigger
> - * used for this device to be specified at run time based on the trigger's
> - * name.
> - *
> - * Return: negative error code on failure or length of the buffer
> - *	   on success
> - */
> -static ssize_t current_trigger_store(struct device *dev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t len)
> +/* Must be called with iio_dev_opaque->mlock held. */
> +static ssize_t iio_trigger_set_current(struct iio_dev *indio_dev,
> +				       const char *buf, size_t len)
>  {
> -	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 *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;
> -	}
> -
>  	trig = iio_trigger_acquire_by_name(buf);
>  	if (oldtrig == trig) {
>  		ret = len;
> @@ -492,6 +469,37 @@ out_trigger_put:
>  	return ret;
>  }
>  
> +/**
> + * current_trigger_store() - trigger consumer sysfs set current trigger
> + * @dev:	device associated with an industrial I/O device
> + * @attr:	device attribute that is being processed
> + * @buf:	string buffer that holds the name of the trigger
> + * @len:	length of the trigger name held by buf
> + *
> + * For trigger consumers the current_trigger interface allows the trigger
> + * used for this device to be specified at run time based on the trigger's
> + * name.
> + *
> + * Return: negative error code on failure or length of the buffer
> + *	   on success
> + */
> +static ssize_t current_trigger_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> +	guard(mutex)(&iio_dev_opaque->mlock);
> +
> +	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED)
> +		return -EBUSY;
> +	if (iio_dev_opaque->trig_readonly)
> +		return -EPERM;
> +
> +	return iio_trigger_set_current(indio_dev, buf, len);
> +}
> +
>  static DEVICE_ATTR_RW(current_trigger);
>  
>  static struct attribute *iio_trigger_consumer_attrs[] = {
> ---
> v2:
>  - Move the trigger swap into a helper, iio_trigger_set_current(), and
>    take mlock with guard() in current_trigger_store() only, so that
>    guard() and the goto based cleanup of the newly acquired trigger are
>    not mixed in one function (Jonathan Cameron, Sashiko). No change to
>    the locking itself.
> v1: https://lore.kernel.org/all/20261026100246.68005-1-yuz08559@gmail.com/
>