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

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

[Carlo Szelinsky]

This series fixes use-after-free bugs in the PSE core teardown path.

When a PSE controller is unregistered, pse_controller_unregister() frees
the PI array (pcdev->pi) and the power domain supplies. But the IRQ
handler, the notification worker, and the regulator disable path can
still run at that point and touch this freed memory.

Patch 1 reorders pse_controller_unregister() so the IRQ and the worker
are stopped before anything they use is freed.

Patch 2 clears pcdev->pi under the lock after freeing it, and makes the
three regulator ops that read pcdev->pi return early when it is NULL.

This series closes the use-after-free reachable from the regulator core
and the controller's own IRQ and worker during unregister. The wider
case - a pse_control consumer (the ethtool path) whose handle outlives
the controller - is a separate lifetime problem handled by the PSE
notifier decoupling work for net-next, not this fix.

Both bugs are pre-existing. They are teardown races, so there is no easy
way to trigger them on purpose and no simple reproducer. The fix is based
on code review. It is compile tested and checkpatch clean.

v1: https://lore.kernel.org/all/20260524223306.2570676-1-github@szelinsky.de/

Changes in v2:
- Patch 1: also stop the IRQ before pse_flush_pw_ds(), and cancel the
  notification worker before pse_release_pis(). v1 only moved
  disable_irq() ahead of pse_release_pis(). Also fix the commit message,
  which wrongly said the worker does not touch pcdev->pi.
- Patch 2: take pcdev->lock around the kfree() and the pcdev->pi = NULL
  store, so a reader sees an authoritative NULL. Add the same NULL guard
  to pse_pi_enable() and pse_pi_is_enabled(), not just pse_pi_disable().
- Thanks to Simon Horman for the review.

Carlo Szelinsky (2):
  net: pse-pd: stop async event sources before freeing PI data in
    unregister
  net: pse-pd: guard regulator ops against freed PI data during
    unregister

 drivers/net/pse-pd/pse_core.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH net v2 1/2] net: pse-pd: stop async event sources before freeing PI data in unregister

[Carlo Szelinsky]

pse_controller_unregister() frees resources that its own asynchronous
event sources are still using:

 * The PI array (pcdev->pi) is freed by pse_release_pis() while the
   threaded IRQ handler pse_isr() can still be running. pse_isr() walks
   pcdev->pi[] (via pse_set_config_isr() and
   regulator_notifier_call_chain() on pcdev->pi[i].rdev), so an interrupt
   arriving before disable_irq() dereferences freed memory.

 * pse_flush_pw_ds() runs before disable_irq() and drops the power domain
   references, which can free pw_d->supply via __pse_pw_d_release().
   A concurrent interrupt reaches that supply through
   _pse_pi_disable() -> pse_pw_d_retry_power_delivery() ->
   regulator_request_power_budget(pw_d->supply), another use-after-free.

 * cancel_work_sync(&pcdev->ntf_work) runs after pse_release_pis(), but
   the notification worker reaches pcdev->pi too: pse_send_ntf_worker()
   -> pse_control_put() -> __pse_control_release() dereferences
   psec->pcdev->pi[psec->id].admin_state_enabled. Draining the worker
   after the PI array is freed is therefore also a use-after-free.

Reorder teardown so every asynchronous accessor is stopped first:
disable the IRQ, drain the notification worker, and only then flush the
power domains and release the PI array.

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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index a5e6d7b26b9f..6045b6c399c2 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -1114,11 +1114,15 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
-	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
+	/* Stop the IRQ and notification worker before freeing what they
+	 * reach: both touch pcdev->pi, and the IRQ also uses pw_d->supply
+	 * that pse_flush_pw_ds() drops.
+	 */
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
 	cancel_work_sync(&pcdev->ntf_work);
+	pse_flush_pw_ds(pcdev);
+	pse_release_pis(pcdev);
 	kfifo_free(&pcdev->ntf_fifo);
 	mutex_lock(&pse_list_mutex);
 	list_del(&pcdev->list);
-- 
2.43.0

[PATCH net v2 2/2] net: pse-pd: guard regulator ops against freed PI data during unregister

[Carlo Szelinsky]

The 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_pi_enable()
and pse_pi_is_enabled() dereference pcdev->pi[id] the same way and are
reachable by any regulator consumer that keeps a handle across the
teardown window.

Clear pcdev->pi after freeing it and bail out of the three regulator ops
that dereference it when it is NULL. Perform the kfree() and NULL store in
pse_release_pis() under pcdev->lock, and read pcdev->pi under the same lock
in the ops, so the NULL an op observes is authoritative even when the free
runs concurrently on another CPU: the op either sees the live array or
returns without touching freed memory.

The other three regulator ops (pse_pi_get_voltage(),
pse_pi_get_current_limit(), pse_pi_set_current_limit()) do not dereference
pcdev->pi and need no guard.

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 | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 6045b6c399c2..21ccb5146616 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -144,7 +144,13 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)
 		of_node_put(pcdev->pi[i].pairset[1].np);
 		of_node_put(pcdev->pi[i].np);
 	}
+	/* Free under the lock so the NULL store is authoritative against
+	 * the regulator ops that read pcdev->pi under pcdev->lock.
+	 */
+	mutex_lock(&pcdev->lock);
 	kfree(pcdev->pi);
+	pcdev->pi = NULL;
+	mutex_unlock(&pcdev->lock);
 }
 
 /**
@@ -421,6 +427,11 @@ static int pse_pi_is_enabled(struct regulator_dev *rdev)
 
 	id = rdev_get_id(rdev);
 	mutex_lock(&pcdev->lock);
+	/* Controller may be unregistered (pcdev->pi freed) mid-teardown. */
+	if (!pcdev->pi) {
+		ret = -ENODEV;
+		goto out;
+	}
 	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
 		ret = pcdev->pi[id].admin_state_enabled;
 		goto out;
@@ -674,6 +685,11 @@ static int pse_pi_enable(struct regulator_dev *rdev)
 
 	id = rdev_get_id(rdev);
 	mutex_lock(&pcdev->lock);
+	/* Controller may be unregistered (pcdev->pi freed) mid-teardown. */
+	if (!pcdev->pi) {
+		mutex_unlock(&pcdev->lock);
+		return -ENODEV;
+	}
 	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
 		/* Manage enabled status by software.
 		 * Real enable process will happen if a port is connected.
@@ -702,15 +718,20 @@ 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);
+	/* Reached via the regulator core's deferred-disable flush after
+	 * pcdev->pi is freed on unregister.
+	 */
+	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 v2 1/2] net: pse-pd: stop async event sources before freeing PI data in unregister

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260711121611.1639086-2-github@szelinsky.de?part=1
---
--- Patch [1]: [PATCH net v2 1/2] net: pse-pd: stop async event sources before freeing PI data in unregister ---
commit bc4c653b9b2b42b2c22ca8b6cae3af394c8add0e
Author: Carlo Szelinsky <github@szelinsky.de>

net: pse-pd: stop async event sources before freeing PI data in unregister

This commit reorders the teardown process in pse_controller_unregister().
It disables the IRQ and flushes the notification worker before flushing power
domains and releasing the PI array. This is intended to prevent use-after-free
issues caused by asynchronous accessors referencing freed data structures.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index a5e6d7b26b9fd..6045b6c399c26 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1114,11 +1114,15 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
>   */
>  void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  {
> -	pse_flush_pw_ds(pcdev);
> -	pse_release_pis(pcdev);
> +	/* Stop the IRQ and notification worker before freeing what they
> +	 * reach: both touch pcdev->pi, and the IRQ also uses pw_d->supply
> +	 * that pse_flush_pw_ds() drops.
> +	 */
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);

[Severity: High]
Does this code call disable_irq() on an already freed IRQ line?

In tps23881.c, devm_pse_controller_register() is called before
tps23881_setup_irq(). Because devres executes teardown actions in LIFO order,
the IRQ is freed before pse_controller_unregister() runs.

Calling disable_irq() here operates on a freed IRQ descriptor, which can cause
warnings or corrupt state.

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

[Severity: High]
This isn't a bug introduced by this patch, but releasing the PIs here causes a
use-after-free via sysfs.

In pse_controller_register(), PI regulators are registered via
devm_pse_pi_regulator_register() before devm_pse_controller_register() is called
by the driver. Devres LIFO ordering means pse_controller_unregister() runs
first, freeing pcdev->pi.

The regulators remain active until their devres actions run later.

If a user space read via sysfs triggers pse_pi_is_enabled() during this window,
it will dereference the freed pcdev->pi array.

[Severity: High]
This is a pre-existing issue, but does freeing the power domains here create a
use-after-free for shared domains?

In pse_register_pw_ds(), shared power domains are added to a global pse_pw_d_map
and their refcount is incremented, but they are allocated with devm_kzalloc()
tied to a specific device.

When the original allocating device is unbound, devres frees the memory, leaving
dangling pointers in the global map and in any other device sharing the domain.

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

[Severity: High]
This is a pre-existing issue, but does destroying internal data structures
before removing the controller from the global list create a use-after-free?

Because pse_release_pis() runs before list_del(), a concurrent device probe
calling of_pse_control_get() can iterate over pse_controller_list, call
of_pse_match_pi(), and dereference the already-freed pcdev->pi[i].np.

[Severity: High]
This is a pre-existing issue, but the PSE control references don't seem to
manage object lifetime properly.

When a consumer gets a control via of_pse_control_get(), it takes a module
reference but does not establish a device link enforcing that the supplier
outlives the consumer.

If the PSE controller is unbound via sysfs, this teardown path frees pcdev->pi.
The active consumer can then dereference freed memory when it later calls
__pse_control_release().

[Severity: Medium]
This is a pre-existing issue, but does pse_controller_register() leak memory on
error paths?

If setup steps fail after allocating the kfifo and the PI structures (e.g.,
missing callbacks or setup_pi_matrix() failures), the function returns the
error without calling kfifo_free() or pse_release_pis().

Re: [PATCH net v2 2/2] net: pse-pd: guard regulator ops against freed PI data during unregister

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260711121611.1639086-2-github@szelinsky.de?part=2
---
--- Patch [2]: [PATCH net v2 2/2] net: pse-pd: guard regulator ops against freed PI data during unregister ---
commit 651be76b54426e6621f4330232053b29d0731f5d
Author: Carlo Szelinsky <github@szelinsky.de>

net: pse-pd: guard regulator ops against freed PI data during unregister

The commit tries to fix a use-after-free by clearing pcdev->pi to NULL under
pcdev->lock during unregister, and adding NULL checks in the regulator
operations.

[Severity: High]
This is a pre-existing issue, but could the entire pcdev object be used after
free because the API fails to establish a stateful device link between the PSE
controller and the consumer?

If the supplier driver is unbound (e.g., via sysfs or hot-unplug), the memory
backing pcdev is freed via devres. Because of_pse_control_get() acquires the
control without adding a device link (device_link_add) to ensure the supplier
cannot be unbound:

of_pse_control_get()
    psec = pse_control_get_internal(pcdev, psec_id, phydev);

The network device continues to hold a dangling psec->pcdev pointer, and
subsequent ethtool requests dereference the freed pcdev:

pse_ethtool_get_status()
    pcdev = psec->pcdev;
    ops = pcdev->ops;

[Severity: High]
This is a pre-existing issue, but does error handling in pse_register_pw_ds()
leak xa_alloc entries, leading to a use-after-free on a subsequent driver
reload?

If pse_register_pw_ds() succeeds in allocating a power domain via
devm_pse_alloc_pw_d():

devm_pse_alloc_pw_d()
    ret = xa_alloc(&pse_pw_d_map, &index, pw_d, XA_LIMIT(1,
                   PSE_PW_D_LIMIT), GFP_KERNEL);

But fails on a later iteration, it bails out without cleaning up previously
allocated and mapped pw_d instances:

pse_register_pw_ds()
    pw_d = devm_pse_alloc_pw_d(pcdev->dev);
    if (IS_ERR(pw_d)) {
        ret = PTR_ERR(pw_d);
        goto out;
    }

The devm-managed pointer remains in the global xarray after devres frees it.
The next time the driver probes, xa_for_each(&pse_pw_d_map, ...) will access
the freed pw_d->supply pointer.

[Severity: High]
Does this patch series introduce a disable_irq() operation on an already
freed IRQ during devres unwind?

Devres unwinds resources in LIFO order. In drivers like tps23881.c, the devres
order is established where the controller is registered before the IRQ is
requested:

tps23881_i2c_probe()
    ret = devm_pse_controller_register(dev, &priv->pcdev);
    if (ret) {
        return dev_err_probe(dev, ret, "failed to register PSE controller\n");
    }
    
    ret = tps23881_setup_irq(priv, client->irq);

On driver detach, the IRQ is freed first. Then devm_pse_controller_release()
runs and calls pse_controller_unregister(), which calls disable_irq() on an
IRQ that was already freed:

pse_controller_unregister()
    if (pcdev->irq)
        disable_irq(pcdev->irq);

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 6045b6c399c26..21ccb51466160 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -144,7 +144,13 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)
>  		of_node_put(pcdev->pi[i].pairset[1].np);
>  		of_node_put(pcdev->pi[i].np);
>  	}
> +	/* Free under the lock so the NULL store is authoritative against
> +	 * the regulator ops that read pcdev->pi under pcdev->lock.
> +	 */
> +	mutex_lock(&pcdev->lock);
>  	kfree(pcdev->pi);
> +	pcdev->pi = NULL;

[Severity: High]
Does setting pcdev->pi to NULL without guarding consumer paths convert a
use-after-free into a deterministic NULL pointer dereference?

For example, a network consumer holding a pse_control reference can call
ethtool status, which evaluates the pointer from the NULL pcdev->pi and
dereferences it:

pse_ethtool_get_status()
    pi = &pcdev->pi[psec->id];
    mutex_lock(&pcdev->lock);
    if (pi->pw_d) {

Similarly, __pse_control_release() unconditionally accesses pcdev->pi
completely unguarded:

__pse_control_release()
    if (psec->pcdev->pi[psec->id].admin_state_enabled)
        regulator_disable(psec->ps);


[Severity: High]
Could freeing and NULLing pcdev->pi here while the controller is still
globally visible cause a race condition with of_pse_control_get()?

pse_controller_unregister() calls pse_release_pis(), and only later removes
the controller from the globally visible list:

pse_controller_unregister()
    pse_release_pis(pcdev);
    kfifo_free(&pcdev->ntf_fifo);
    mutex_lock(&pse_list_mutex);
    list_del(&pcdev->list);

Concurrently, of_pse_control_get() can iterate the list and call
of_pse_match_pi(), which dereferences the NULL pcdev->pi array:

of_pse_match_pi()
    for (i = 0; i < pcdev->nr_lines; i++) {
        if (pcdev->pi[i].np == np)

> +	mutex_unlock(&pcdev->lock);
>  }
>  
>  /**

[ ... ]

> @@ -702,15 +718,20 @@ 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);

[Severity: High]
This is a pre-existing issue, but is there a race condition between
pse_flush_pw_ds() and pse_pi_disable() that causes a use-after-free of the
regulator supply?

pse_flush_pw_ds() drops the power domain refcount, which leads to
regulator_put() freeing the regulator object. This is done without holding
pcdev->lock:

pse_flush_pw_ds()
    kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release, &pse_pw_d_mutex);

Concurrently, if the regulator core's delayed disable worker invokes
pse_pi_disable(), it can see a non-NULL pcdev->pi, proceed to call
_pse_pi_disable(), and eventually call pse_pi_deallocate_pw_budget().

pse_pi_deallocate_pw_budget() then attempts to dereference the freed regulator
supply:

pse_pi_deallocate_pw_budget()
    regulator_free_power_budget(pi->pw_d->supply, pi->pw_allocated_mW);

> +	/* Reached via the regulator core's deferred-disable flush after
> +	 * pcdev->pi is freed on unregister.
> +	 */
> +	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;