[PATCH v3 1/2] iio: accel: kxsd9: fix Use-After-Free in remove()

[PATCH v3 1/2] iio: accel: kxsd9: fix Use-After-Free in remove()

[Biren Pandya]

The kxsd9 driver currently calls iio_triggered_buffer_cleanup() before
iio_device_unregister() in the remove() function. This order creates a
race condition where userspace can still access sysfs or ioctl interfaces
while the triggered buffers are being torn down, potentially leading to
a use-after-free.

Fix this by swapping the cleanup order. Unregister the IIO device first
to guarantee that all userspace interfaces are destroyed and no new
accesses can occur before cleaning up the triggered buffers.

This vulnerability was flagged by the Sashiko automated review system.

Link: https://sashiko.dev/#/patchset/20260621193036.78549-2-birenpandya@gmail.com
Fixes: 0427a106a98a ("iio: accel: kxsd9: Add triggered buffer handling")
Cc: stable@vger.kernel.org
Signed-off-by: Biren Pandya <birenpandya@gmail.com>
---
 drivers/iio/accel/kxsd9.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 4717d80fc24af..7569201ed3c75 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -477,8 +477,8 @@ void kxsd9_common_remove(struct device *dev)
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct kxsd9_state *st = iio_priv(indio_dev);
 
-	iio_triggered_buffer_cleanup(indio_dev);
 	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
 	pm_runtime_get_sync(dev);
 	pm_runtime_put_noidle(dev);
 	pm_runtime_disable(dev);
-- 
2.50.1 (Apple Git-155)

[PATCH v3 2/2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[Biren Pandya]

The kxsd9 driver previously used manual pm_runtime_get_sync() and
pm_runtime_put_autosuspend() calls in various return paths, which is
prone to memory leaks if a return path is missed. Specifically, in
kxsd9_write_raw(), if the user provides a non-zero integer component
for scale, the function previously returned -EINVAL directly, leaking
the runtime PM usage counter.

Switch to using the new scoped-based PM_RUNTIME_ACQUIRE_AUTOSUSPEND()
and PM_RUNTIME_ACQUIRE_ERR() macros to automatically handle dropping
the PM usage counter upon return. This simplifies the logic by allowing
early returns in error paths.

In remove(), use pm_runtime_resume_and_get() with an error check to
prevent unchecked return issues, and perform a best-effort power down.

Fixes: 9a9a369d6178 ("iio: accel: kxsd9: Deploy system and runtime PM")
Signed-off-by: Biren Pandya <birenpandya@gmail.com>
---
 drivers/iio/accel/kxsd9.c | 59 ++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index 7569201ed3c75..a71b473cbfa03 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -139,68 +139,65 @@ static int kxsd9_write_raw(struct iio_dev *indio_dev,
 			   int val2,
 			   long mask)
 {
-	int ret = -EINVAL;
 	struct kxsd9_state *st = iio_priv(indio_dev);
+	int ret;
 
-	pm_runtime_get_sync(st->dev);
+	if (mask != IIO_CHAN_INFO_SCALE)
+		return -EINVAL;
 
-	if (mask == IIO_CHAN_INFO_SCALE) {
-		/* Check no integer component */
-		if (val)
-			return -EINVAL;
-		ret = kxsd9_write_scale(indio_dev, val2);
-	}
+	if (val)
+		return -EINVAL;
 
-	pm_runtime_put_autosuspend(st->dev);
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(st->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
 
-	return ret;
+	return kxsd9_write_scale(indio_dev, val2);
 }
 
 static int kxsd9_read_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan,
 			  int *val, int *val2, long mask)
 {
-	int ret = -EINVAL;
 	struct kxsd9_state *st = iio_priv(indio_dev);
 	unsigned int regval;
 	__be16 raw_val;
 	u16 nval;
+	int ret;
 
-	pm_runtime_get_sync(st->dev);
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(st->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = regmap_bulk_read(st->map, chan->address, &raw_val,
 				       sizeof(raw_val));
 		if (ret)
-			goto error_ret;
+			return ret;
 		nval = be16_to_cpu(raw_val);
 		/* Only 12 bits are valid */
 		nval >>= 4;
 		*val = nval;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
 		/* This has a bias of -2048 */
 		*val = KXSD9_ZERO_G_OFFSET;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		ret = regmap_read(st->map,
 				  KXSD9_REG_CTRL_C,
 				  &regval);
 		if (ret < 0)
-			goto error_ret;
+			return ret;
 		*val = 0;
 		*val2 = kxsd9_micro_scales[regval & KXSD9_CTRL_C_FS_MASK];
-		ret = IIO_VAL_INT_PLUS_MICRO;
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 
-error_ret:
-	pm_runtime_put_autosuspend(st->dev);
-
-	return ret;
+	return -EINVAL;
 };
 
 static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
@@ -239,9 +236,7 @@ static int kxsd9_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct kxsd9_state *st = iio_priv(indio_dev);
 
-	pm_runtime_get_sync(st->dev);
-
-	return 0;
+	return pm_runtime_resume_and_get(st->dev);
 }
 
 static int kxsd9_buffer_postdisable(struct iio_dev *indio_dev)
@@ -476,12 +471,18 @@ void kxsd9_common_remove(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct kxsd9_state *st = iio_priv(indio_dev);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
-	pm_runtime_get_sync(dev);
-	pm_runtime_put_noidle(dev);
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		dev_warn(dev, "Failed to resume device (%d)\n", ret);
+
 	pm_runtime_disable(dev);
+	if (ret >= 0)
+		pm_runtime_put_noidle(dev);
 	kxsd9_power_down(st);
 }
 EXPORT_SYMBOL_NS(kxsd9_common_remove, "IIO_KXSD9");
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v3 1/2] iio: accel: kxsd9: fix Use-After-Free in remove()

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 01:16:51PM +0530, Biren Pandya wrote:
> The kxsd9 driver currently calls iio_triggered_buffer_cleanup() before
> iio_device_unregister() in the remove() function. This order creates a
> race condition where userspace can still access sysfs or ioctl interfaces
> while the triggered buffers are being torn down, potentially leading to
> a use-after-free.
> 
> Fix this by swapping the cleanup order. Unregister the IIO device first
> to guarantee that all userspace interfaces are destroyed and no new
> accesses can occur before cleaning up the triggered buffers.
> 
> This vulnerability was flagged by the Sashiko automated review system.

> Link: https://sashiko.dev/#/patchset/20260621193036.78549-2-birenpandya@gmail.com

I believe you wanted Closes: and Reported-by: tags instead of simple Link.

> Fixes: 0427a106a98a ("iio: accel: kxsd9: Add triggered buffer handling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Biren Pandya <birenpandya@gmail.com>

...

You have a series, where is the cover letter and the changelog, please?
Do not abuse the process.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 01:16:52PM +0530, Biren Pandya wrote:
> The kxsd9 driver previously used manual pm_runtime_get_sync() and
> pm_runtime_put_autosuspend() calls in various return paths, which is
> prone to memory leaks if a return path is missed. Specifically, in
> kxsd9_write_raw(), if the user provides a non-zero integer component
> for scale, the function previously returned -EINVAL directly, leaking
> the runtime PM usage counter.
> 
> Switch to using the new scoped-based PM_RUNTIME_ACQUIRE_AUTOSUSPEND()
> and PM_RUNTIME_ACQUIRE_ERR() macros to automatically handle dropping
> the PM usage counter upon return. This simplifies the logic by allowing
> early returns in error paths.
> 
> In remove(), use pm_runtime_resume_and_get() with an error check to
> prevent unchecked return issues, and perform a best-effort power down.

...

> void kxsd9_common_remove(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct kxsd9_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
> -	pm_runtime_get_sync(dev);
> -	pm_runtime_put_noidle(dev);
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		dev_warn(dev, "Failed to resume device (%d)\n", ret);
> +
>  	pm_runtime_disable(dev);
> +	if (ret >= 0)
> +		pm_runtime_put_noidle(dev);

Has this been discussed already? Because it looks to me a bit strange to have
_disable() be called before _put_noidle(). Also the commit message is unclear
about the flow change.

>  	kxsd9_power_down(st);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[Jonathan Cameron]

On Mon,  6 Jul 2026 13:16:52 +0530
Biren Pandya <birenpandya@gmail.com> wrote:

> The kxsd9 driver previously used manual pm_runtime_get_sync() and
> pm_runtime_put_autosuspend() calls in various return paths, which is
> prone to memory leaks if a return path is missed. Specifically, in
> kxsd9_write_raw(), if the user provides a non-zero integer component
> for scale, the function previously returned -EINVAL directly, leaking
> the runtime PM usage counter.

This has become too complex. For the fix, can we just close that one
problem case.  Then follow up with the refactors and hardening after?

The minimal fix is probably the code move you have to check that
the request is valid before doing anything with runtime pm.

> 
> Switch to using the new scoped-based PM_RUNTIME_ACQUIRE_AUTOSUSPEND()
> and PM_RUNTIME_ACQUIRE_ERR() macros to automatically handle dropping
> the PM usage counter upon return. This simplifies the logic by allowing
> early returns in error paths.
> 
> In remove(), use pm_runtime_resume_and_get() with an error check to
> prevent unchecked return issues, and perform a best-effort power down.
> 
> Fixes: 9a9a369d6178 ("iio: accel: kxsd9: Deploy system and runtime PM")
> Signed-off-by: Biren Pandya <birenpandya@gmail.com>
> ---
>  drivers/iio/accel/kxsd9.c | 59 ++++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index 7569201ed3c75..a71b473cbfa03 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -139,68 +139,65 @@ static int kxsd9_write_raw(struct iio_dev *indio_dev,
>  			   int val2,
>  			   long mask)
>  {
> -	int ret = -EINVAL;
>  	struct kxsd9_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -	pm_runtime_get_sync(st->dev);
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
>  
> -	if (mask == IIO_CHAN_INFO_SCALE) {
> -		/* Check no integer component */
> -		if (val)
> -			return -EINVAL;
> -		ret = kxsd9_write_scale(indio_dev, val2);
> -	}
> +	if (val)
> +		return -EINVAL;
>  
> -	pm_runtime_put_autosuspend(st->dev);
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(st->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	return kxsd9_write_scale(indio_dev, val2);
Yikes this driver has 'aged'.  This call has a nasty
goto err_ret in one path that just returns the error, yet
does direct retuns in the other.

Anyhow, the reason I mention that is an alternative
to this would be to move the runtime pm into
kxsd9_write_scale()  That would be part of a more signficant
cleanup but if you are interested it... Lets just take that function:

> static int kxsd9_write_scale(struct iio_dev *indio_dev, int micro)
> {
> 	int ret, i;
> 	struct kxsd9_state *st = iio_priv(indio_dev);
> 	bool foundit = false;

After changes below can also do a bit of reordering to make this
reverse xmas tree.

> 
> 	for (i = 0; i < 4; i++)
Replace the 4 with ARRAY_SIZE(kxsd9_micro_scales) ou
and 
	for (unsigned int i = 0; i < ARRAY_SIZE(kxsd9_micro_scales), i++)

> 		if (micro == kxsd9_micro_scales[i]) {
> 			foundit = true;
Drop foundit as pointless. Instead...
> 			break;
> 		}
> 	if (!foundit)
	if (i == ARRAY_SIZE(kxsd9_micro_scales))
		return -EINVAL;

> 		return -EINVAL;
> 
> 	ret = regmap_update_bits(st->map,
> 				 KXSD9_REG_CTRL_C,
> 				 KXSD9_CTRL_C_FS_MASK,
> 				 i);
rewrap that.
> 	if (ret < 0)
> 		goto error_ret;
	if (ret)

As regmap always returns <= 0 

		return ret; 

> 
> 	/* Cached scale when the sensor is powered down */

Not sure this is terribly helpful as comments go, so maybe drop it.

> 	st->scale = i;
> 
> error_ret:
> 	return ret;

return 0;

> }

I don't have time now to do a full driver review (too many patches
waiting) but in general this looks like it could do with some
'modernization'.   I feel justified in being particularly rude
about this one as a much younger me wrote it :)

>  }
>  
>  static int kxsd9_read_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan,
>  			  int *val, int *val2, long mask)
>  {
> -	int ret = -EINVAL;
>  	struct kxsd9_state *st = iio_priv(indio_dev);
>  	unsigned int regval;
>  	__be16 raw_val;
>  	u16 nval;
> +	int ret;
>  
> -	pm_runtime_get_sync(st->dev);
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(st->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = regmap_bulk_read(st->map, chan->address, &raw_val,
>  				       sizeof(raw_val));
>  		if (ret)
> -			goto error_ret;
> +			return ret;
>  		nval = be16_to_cpu(raw_val);
>  		/* Only 12 bits are valid */
>  		nval >>= 4;
>  		*val = nval;
> -		ret = IIO_VAL_INT;
> -		break;
> +		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_OFFSET:
>  		/* This has a bias of -2048 */
>  		*val = KXSD9_ZERO_G_OFFSET;
> -		ret = IIO_VAL_INT;
> -		break;
> +		return IIO_VAL_INT;

Hmm. Turning the power on to output a known constant is a bit
ugly and a side effect of the original goto pattern.  Let us
drag the PM_RUNTIME_ACQUIRE... into the cases that need it.
Make sure to use {} to define scope for case blocks where you
are doing this.

>  	case IIO_CHAN_INFO_SCALE:
>  		ret = regmap_read(st->map,
>  				  KXSD9_REG_CTRL_C,
>  				  &regval);
>  		if (ret < 0)
> -			goto error_ret;
> +			return ret;
>  		*val = 0;
>  		*val2 = kxsd9_micro_scales[regval & KXSD9_CTRL_C_FS_MASK];
> -		ret = IIO_VAL_INT_PLUS_MICRO;
> -		break;
> +		return IIO_VAL_INT_PLUS_MICRO;

	default:
		reutrn -EINVAL;
and get rid of the block below.   The reason for this style is to
make it obvious it is intentional that the switch statement only
covers a subset.

>  	}
>  
> -error_ret:
> -	pm_runtime_put_autosuspend(st->dev);
> -
> -	return ret;
> +	return -EINVAL;
>  };
>  
>  static irqreturn_t kxsd9_trigger_handler(int irq, void *p)
> @@ -239,9 +236,7 @@ static int kxsd9_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct kxsd9_state *st = iio_priv(indio_dev);
>  
> -	pm_runtime_get_sync(st->dev);
> -
> -	return 0;
> +	return pm_runtime_resume_and_get(st->dev);
>  }
>  
>  static int kxsd9_buffer_postdisable(struct iio_dev *indio_dev)
> @@ -476,12 +471,18 @@ void kxsd9_common_remove(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct kxsd9_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
> -	pm_runtime_get_sync(dev);
> -	pm_runtime_put_noidle(dev);
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		dev_warn(dev, "Failed to resume device (%d)\n", ret);
> +
>  	pm_runtime_disable(dev);
> +	if (ret >= 0)
> +		pm_runtime_put_noidle(dev);
Andy's point is valid about the reorder.  I don't think it is
a bug, but I'm fairly sure it is unnecessary as nothing else is
going to mess with the reference count in this window.

I'm also not sure I care about an underflow if we are in the
weird corners of runtime pm failing on remove.
Note that it doesn't actually fail if you drop it too far (despite
what sashiko thinks).  Runtime pm counters are always >= 0.
Implementation is:
static inline void pm_runtime_put_noidle(struct device *dev)
{
	atomic_add_unless(&dev->power.usage_count, -1, 0);
}

Whilst I prefer to keep balance, that protection is there to
simplify this sort of tear down triggered underflow so let us
use it here.


>  	kxsd9_power_down(st);
>  }
>  EXPORT_SYMBOL_NS(kxsd9_common_remove, "IIO_KXSD9");

Re: [PATCH v3 2/2] iio: accel: kxsd9: fix runtime PM leaks and unchecked returns

[biren pandya]

On Sun, Jul 12, 2026 at 4:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon,  6 Jul 2026 13:16:52 +0530
> Biren Pandya <birenpandya@gmail.com> wrote:
>
> > The kxsd9 driver previously used manual pm_runtime_get_sync() and
> > pm_runtime_put_autosuspend() calls in various return paths, which is
> > prone to memory leaks if a return path is missed. Specifically, in
> > kxsd9_write_raw(), if the user provides a non-zero integer component
> > for scale, the function previously returned -EINVAL directly, leaking
> > the runtime PM usage counter.
>
> This has become too complex. For the fix, can we just close that one
> problem case.  Then follow up with the refactors and hardening after?
>
> The minimal fix is probably the code move you have to check that
> the request is valid before doing anything with runtime pm.

Thanks Jonathan, Andy — agreed on all of it.

I've just sent out v4 as a fresh thread. It contains just the minimal
leak fix: moving the mask/val validation ahead of
pm_runtime_get_sync() in write_raw()
so the -EINVAL paths stop leaking the usage counter. Nothing else is
in that patch.

> I don't have time now to do a full driver review (too many patches
> waiting) but in general this looks like it could do with some
> 'modernization'.   I feel justified in being particularly rude
> about this one as a much younger me wrote it :)

The macro conversion, the per-case PM scoping in read_raw(), and the
write_scale() cleanup (ARRAY_SIZE, drop foundit, return 0,
reverse-xmas-tree) I will send as a separate follow-up series once the
fix is in, so it doesn't hold up the bug.

> Andy's point is valid about the reorder.  I don't think it is
> a bug, but I'm fairly sure it is unnecessary as nothing else is
> going to mess with the reference count in this window.
>
> I'm also not sure I care about an underflow if we are in the
> weird corners of runtime pm failing on remove.
> Note that it doesn't actually fail if you drop it too far (despite
> what sashiko thinks).  Runtime pm counters are always >= 0.

I've dropped the remove() rework entirely from v4 — you're both right
that put_noidle() floors at 0 natively,
so the reorder is unnecessary and the underflow concern wasn't real.
Still learning exactly where the line sits between "fix" and "cleanup"
for stable.
Thanks for the patience and the detailed checklist for the upcoming
modernization series!