[PATCH net 0/2] net: pse-pd: fix use-after-free of PI array on controller teardown

[PATCH net 0/2] net: pse-pd: fix use-after-free of PI array on controller teardown

[Carlo Szelinsky]

Two pre-existing use-after-frees in the PSE core teardown path surfaced
during review of the v5 poll/LED series: the IRQ-vs-pse_release_pis()
ordering was raised as an open question in the v5 cover, and the
regulator-disable UAF was spotted during Jakub's review of the LED
changes. They are independent of the poll/LED feature work, so as
suggested on the list they are sent here to net on their own.

Both are reached on controller unregister / driver unbind:

Patch 1: pse_controller_unregister() frees the PI array via
pse_release_pis() before disabling the IRQ, so a threaded pse_isr()
firing in that window walks the freed pcdev->pi[].

Patch 2: the PI regulators are devm-registered inside
pse_controller_register(), so on unbind devres tears the controller
down (freeing pcdev->pi) before the regulators. A deferred disable
flushed during regulator_unregister() then dereferences the freed PI
array in pse_pi_disable().

Both carry the same Fixes: tag (ffef61d6d273). The v6 poll/LED series
will be posted to net-next once these land and net-next has merged
them.

Link: https://lore.kernel.org/all/20260429213224.1747410-1-github@szelinsky.de/

Carlo Szelinsky (2):
  net: pse-pd: disable IRQ before freeing PI data in unregister
  net: pse-pd: guard against freed PI data on regulator disable

 drivers/net/pse-pd/pse_core.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH net 1/2] net: pse-pd: disable IRQ before freeing PI data in unregister

[Carlo Szelinsky]

pse_controller_unregister() frees the PI array via pse_release_pis()
before disabling the controller IRQ. The threaded IRQ handler pse_isr()
walks pcdev->pi[] (via pse_set_config_isr() and
regulator_notifier_call_chain() on pcdev->pi[i].rdev), so an interrupt
arriving in the window between pse_release_pis() and disable_irq()
dereferences freed memory.

Disable the IRQ first, then release the PI array. cancel_work_sync()
for the notification worker stays after pse_release_pis(): the worker
only touches the kfifo and the pse_control list, not pcdev->pi.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 87aa4f4e9724..17f45e4b672b 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -1115,9 +1115,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
 	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
+	pse_release_pis(pcdev);
 	cancel_work_sync(&pcdev->ntf_work);
 	kfifo_free(&pcdev->ntf_fifo);
 	mutex_lock(&pse_list_mutex);
-- 
2.43.0

Re: [PATCH net 1/2] net: pse-pd: disable IRQ before freeing PI data in unregister

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: pse-pd: disable IRQ before freeing PI data in unregister

pse_controller_unregister() frees the PI array via pse_release_pis()
before disabling the controller IRQ. The threaded IRQ handler pse_isr()
walks pcdev->pi[] (via pse_set_config_isr() and
regulator_notifier_call_chain() on pcdev->pi[i].rdev), so an interrupt
arriving in the window between pse_release_pis() and disable_irq()
dereferences freed memory.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 69dbdbde9d71a..1f27884dd4d70 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1115,9 +1115,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
>  void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  {
>  	pse_flush_pw_ds(pcdev);
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);

Simon says: The issue flagged below seems to warrant investigation
            within the context of this patch.

[Severity: High]
This is a pre-existing issue, but does leaving pse_flush_pw_ds() before
disable_irq() risk a use-after-free of a regulator during an interrupt?

pse_flush_pw_ds() drops references to power domains, which can trigger
__pse_pw_d_release() and free the regulator via regulator_put(pw_d->supply).

If an interrupt arrives after this but before the IRQ is disabled:

pse_isr()
  _pse_pi_disable()
    pse_pw_d_retry_power_delivery()
      regulator_request_power_budget(pw_d->supply, ...)

Could this access the freed regulator? Should disable_irq() be the very
first operation in the teardown sequence?

> +	pse_release_pis(pcdev);
>  	cancel_work_sync(&pcdev->ntf_work);

Simon says: Although this is a pre-exiting issue, it does seem to
            warrant investigation within the context of this patch-set.

[Severity: High]
This isn't a bug introduced by this patch, but can calling
cancel_work_sync() after pse_release_pis() cause an indirect
use-after-free of pcdev->pi?

The worker blocks during cancel_work_sync(), and it can call:

pse_send_ntf_worker()
  pse_control_put(psec)
    __pse_control_release()

Since __pse_control_release() explicitly dereferences
psec->pcdev->pi[psec->id].admin_state_enabled, can the worker execute this
release path while or after pcdev->pi is freed by pse_release_pis()?

>  	kfifo_free(&pcdev->ntf_fifo);
>  	mutex_lock(&pse_list_mutex);
>  	list_del(&pcdev->list);
>  	mutex_unlock(&pse_list_mutex);
>  }

Simon says: I believe the remaining issues should be investigated
            in the context of possible follow-up or separate bug
	    fixes.

[Severity: High]
This is a pre-existing issue, but does removing the device from the global
list at the very end of unregister allow concurrent lookups to access
freed memory?

The device remains discoverable via of_pse_control_get() while its
resources are actively being destroyed. If a concurrent thread calls
of_pse_control_get(), it will find the pcdev and call
pse_control_get_internal(), which accesses pcdev->pi[index].

Because pse_release_pis() has already freed pcdev->pi earlier in this
sequence, could this cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but does the unregistration sequence lack
synchronization with external pse_control consumers?

of_pse_control_get() provides a pse_control struct to consumers (like PHY
drivers), using try_module_get() but without incrementing the device
reference count or creating strict device links.

When a PSE controller is unbound, pse_controller_unregister() synchronously
frees pcdev->pi. However, external clients may still hold struct
pse_control references.

When these clients are later removed, they call:

pse_control_put(psec)
  __pse_control_release()
    dereferences psec->pcdev->pi[psec->id]

Since pcdev->pi was already freed, does this cause a use-after-free on
client teardown?

[Severity: High]
This is a pre-existing issue, but does the devm teardown sequence free
internal structures while the PSE PI regulators remain fully accessible?

pse_controller_register() registers regulators using
devm_regulator_register(). Afterwards, devm_pse_controller_register()
registers its own release callback.

During driver unbind, the LIFO devm sequence means
devm_pse_controller_release() runs first, freeing pcdev->pi via
pse_release_pis().

However, the regulators are still active and visible in sysfs until the
subsequent devm teardown phase unregisters them. A concurrent sysfs access
(e.g., reading regulator state) during this window will invoke regulator
operations like pse_pi_is_enabled(), which dereference the already freed
pcdev->pi[id].

Could this result in a use-after-free?

[Severity: High]
This is a pre-existing issue, but is the globally shared and
reference-counted struct pse_power_domain object incorrectly tied to the
lifetime of the device that allocated it?

devm_pse_alloc_pw_d() allocates pw_d using devm_kzalloc(). The memory
lifetime is therefore bound to the registering PSE controller. This pw_d is
added to the global pse_pw_d_map and uses kref for reference counting.

pse_register_pw_ds() allows other PSE controllers to discover and share
this pw_d by incrementing its kref.

If the device that originally created pw_d unbinds, devm will unilaterally
free the pw_d memory, ignoring the kref counts held by other devices. Any
subsequent access by those other devices will result in a use-after-free.

The __pse_pw_d_release() function seems to omit kfree(pw_d) because it
relies on devm, but doesn't this break the kref requirement?

[PATCH net 2/2] net: pse-pd: guard against freed PI data on regulator disable

[Carlo Szelinsky]

PSE PI regulators are devm-registered inside pse_controller_register(),
which runs before devres_add() arms the controller's own release in
devm_pse_controller_register(). On driver detach, devres unwinds in LIFO
order, so pse_controller_unregister() runs first and frees pcdev->pi via
pse_release_pis(); the regulators are torn down afterwards.

When regulator_unregister() flushes a pending disable, the regulator core
invokes pse_pi_disable(), which dereferences pcdev->pi[id] (directly and
via _pse_pi_disable() -> pse_pi_deallocate_pw_budget()). At that point the
PI array is already freed, so this is a use-after-free.

pse_release_pis() now clears pcdev->pi after freeing it, and
pse_pi_disable() bails out under the lock when pcdev->pi is NULL, so a
late disable from the regulator core is a no-op once the controller has
been unregistered.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 17f45e4b672b..9ae0df3cb5cf 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -145,6 +145,7 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)
 		of_node_put(pcdev->pi[i].np);
 	}
 	kfree(pcdev->pi);
+	pcdev->pi = NULL;
 }
 
 /**
@@ -702,15 +703,21 @@ static int pse_pi_enable(struct regulator_dev *rdev)
 static int pse_pi_disable(struct regulator_dev *rdev)
 {
 	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
-	struct pse_pi *pi;
 	int id, ret;
 
 	id = rdev_get_id(rdev);
-	pi = &pcdev->pi[id];
 	mutex_lock(&pcdev->lock);
+	/* The controller may already be unregistered (pcdev->pi freed) by the
+	 * time the regulator core flushes a deferred disable during
+	 * regulator_unregister(). Bail out to avoid touching freed PI data.
+	 */
+	if (!pcdev->pi) {
+		mutex_unlock(&pcdev->lock);
+		return 0;
+	}
 	ret = _pse_pi_disable(pcdev, id);
 	if (!ret)
-		pi->admin_state_enabled = 0;
+		pcdev->pi[id].admin_state_enabled = 0;
 
 	mutex_unlock(&pcdev->lock);
 	return 0;
-- 
2.43.0

Re: [PATCH net 2/2] net: pse-pd: guard against freed PI data on regulator disable

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: net: pse-pd: guard against freed PI data on regulator disable

This patch closes a use-after-free reachable from the regulator core's
deferred-disable flush during driver detach by clearing pcdev->pi after
freeing it in pse_release_pis() and bailing out in pse_pi_disable() when
pcdev->pi is NULL.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 1f27884dd4d7..752d987a7004 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -145,6 +145,7 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)
>  		of_node_put(pcdev->pi[i].np);
>  	}
>  	kfree(pcdev->pi);
> +	pcdev->pi = NULL;
>  }

[High]
Is the pcdev->pi = NULL store correctly synchronized with the readers?

The reader side in pse_pi_disable() observes pcdev->pi under
pcdev->lock:

    mutex_lock(&pcdev->lock);
    if (!pcdev->pi) {
            mutex_unlock(&pcdev->lock);
            return 0;
    }

But pse_release_pis() does the kfree() and the NULL store without
acquiring pcdev->lock, and pse_controller_unregister() does not wrap
the pse_release_pis() call with that lock either:

    void pse_controller_unregister(struct pse_controller_dev *pcdev)
    {
            pse_flush_pw_ds(pcdev);
            if (pcdev->irq)
                    disable_irq(pcdev->irq);
            pse_release_pis(pcdev);
            ...
    }

In the LIFO devres scenario described in the commit message, the later
regulator_unregister() does flush_work() on the deferred disable, which
provides the necessary memory barrier, so that specific case is fixed.

However, regulator consumers in other drivers (PHY/MAC) hold a
struct regulator * to a PSE PI and can call regulator_disable() (which
may take the pse_pi_disable() path directly) on a different CPU,
concurrent with the unbind path running pse_release_pis() on this
device.

That concurrent reader can take pcdev->lock and observe pcdev->pi as
still non-NULL while pse_release_pis() is concurrently kfree()-ing the
array, leading to a UAF in the subsequent _pse_pi_disable(pcdev, id)
and pcdev->pi[id].admin_state_enabled = 0 dereferences.

Would taking pcdev->lock around the kfree()+NULL store in
pse_release_pis() (so the NULL observation under pcdev->lock is
authoritative) close that window? Alternatively, can the regulator
unregister be ordered to run before pse_release_pis() so no consumer
can invoke a regulator op while the PI array is being torn down?

> @@ -702,15 +703,21 @@ static int pse_pi_enable(struct regulator_dev *rdev)
>  static int pse_pi_disable(struct regulator_dev *rdev)
>  {
>  	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
> -	struct pse_pi *pi;
>  	int id, ret;
>  
>  	id = rdev_get_id(rdev);
> -	pi = &pcdev->pi[id];
>  	mutex_lock(&pcdev->lock);
> +	/* The controller may already be unregistered (pcdev->pi freed) by the
> +	 * time the regulator core flushes a deferred disable during
> +	 * regulator_unregister(). Bail out to avoid touching freed PI data.
> +	 */
> +	if (!pcdev->pi) {
> +		mutex_unlock(&pcdev->lock);
> +		return 0;
> +	}
>  	ret = _pse_pi_disable(pcdev, id);
>  	if (!ret)
> -		pi->admin_state_enabled = 0;
> +		pcdev->pi[id].admin_state_enabled = 0;
>  
>  	mutex_unlock(&pcdev->lock);
>  	return 0;

[High]
Should the same NULL guard be applied to pse_pi_is_enabled() and
pse_pi_enable()?

Both callbacks follow the identical pattern as the disable path:
rdev_get_drvdata(rdev) -> mutex_lock(&pcdev->lock) -> unconditional
pcdev->pi[id].* access.

In pse_pi_is_enabled():

    id = rdev_get_id(rdev);
    mutex_lock(&pcdev->lock);
    if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
            ret = pcdev->pi[id].admin_state_enabled;
            ...

And in pse_pi_enable():

    id = rdev_get_id(rdev);
    mutex_lock(&pcdev->lock);
    if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
            if (pcdev->pi[id].isr_pd_detected) {
                    ...
            }
            ...

The lifetime invariant cited in the commit message ("the controller
may already be unregistered by the time the regulator core flushes...")
is a property of the LIFO devres unwind order in which
pse_controller_unregister() runs before the devm-registered regulators
are torn down.

While regulator_unregister() only flushes disable_work, regulator
consumers in other drivers that hold a struct regulator * to a PSE PI
can still call regulator_is_enabled() or regulator_enable() concurrent
with the in-progress pse_controller_unregister() on the parent device.
At that point pcdev->pi may already be NULL/freed and the unguarded
pcdev->pi[id].pw_d / pcdev->pi[id].admin_state_enabled dereferences
would be a UAF or NULL deref.

Should these two callbacks receive the same guard, or alternatively
should the unwind order be changed so that no callback can fire after
the PI array is freed?