* Re: [PATCH] iio: trigger: fix use-after-free in current_trigger_store() on concurrent writers
2026-10-26 10:02 [PATCH] iio: trigger: fix use-after-free in current_trigger_store() on concurrent writers Yu Zhang
@ 2026-11-02 0:54 ` Jonathan Cameron
2026-11-02 3:45 ` [PATCH v2] " Yu Zhang
1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2026-11-02 0:54 UTC (permalink / raw)
To: Yu Zhang
Cc: David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, linux-kernel, stable
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;
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v2] iio: trigger: fix use-after-free in current_trigger_store() on concurrent writers
2026-10-26 10:02 [PATCH] iio: trigger: fix use-after-free in current_trigger_store() on concurrent writers Yu Zhang
2026-11-02 0:54 ` Jonathan Cameron
@ 2026-11-02 3:45 ` Yu Zhang
1 sibling, 0 replies; 3+ messages in thread
From: Yu Zhang @ 2026-11-02 3:45 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, linux-kernel, stable
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>
---
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/
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread