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

[PATCH net v3 0/3] 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 while other paths
can still reach them.

To make it clear what the series covers, here is every path that
dereferences pcdev->pi and how each one is made safe:

- The regulator ops pse_pi_enable/disable/is_enabled(). Reached from the
  regulator core and from regulator sysfs (is_enabled() via the "state"
  attribute). The PI regulators are registered before the controller, so
  on unbind the controller is torn down first and pse_release_pis() frees
  pcdev->pi while the regulators are still live. Patch 2 checks
  !pcdev->pi in the three ops and does the free under pcdev->lock.

- of_pse_match_pi(), from of_pse_control_get(). A consumer probing at the
  same time walks pse_controller_list and reads pcdev->pi[i].np. Today
  the PI array is freed before list_del(), so the controller is still on
  the list with pi gone. Patch 3 moves list_del() ahead of the free;
  both run under pse_list_mutex, so a lookup sees either a live pi or no
  controller at all, with no NULL checks on the lookup path.

- pse_ethtool_get_status() and __pse_control_release(), from a consumer
  still holding a pse_control. These read pcdev->pi[psec->id] too, but
  the real issue is that the consumer can outlive the whole controller:
  of_pse_control_get() only takes a module reference, no device link, so
  a NULL check would not help. This is pre-existing and not a net fix.
  It is handled in net-next by the PSE controller notifier series [1],
  where the phy layer drops phydev->psec on unregister before pi is
  freed. phy is the only pse_control consumer in tree.

Patch 1 reorders teardown so the IRQ and the notification worker are
stopped before the power domains and the PI array are freed.

All of these are pre-existing teardown races, so there is no easy way to
trigger them on purpose and no simple reproducer. The fixes are based on
code review. They are compile tested and checkpatch clean.

[1] https://lore.kernel.org/netdev/20260630091125.3162481-1-github@szelinsky.de/

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

Changes in v3:
- New patch 3: unlink the controller from pse_controller_list before
  freeing the PI array, closing the of_pse_control_get() /
  of_pse_match_pi() race.
- Cover letter now maps every pcdev->pi entry point, following the review
  discussion with Kory and Jakub.
- Added Kory Maincent's Reviewed-by to patches 1 and 2.
- Rebased on net/main.

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 (3):
  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
  net: pse-pd: unregister from the controller list before freeing PI
    data

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


base-commit: 3aa1dcaa4f6f5ae08936491e08bd456f331f2d40
-- 
2.43.0

[PATCH net v3 1/3] 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>
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
---
 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 v3 2/3] 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>
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
---
 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 v3 2/3] net: pse-pd: guard regulator ops against freed PI data during unregister

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pse-pd: guard regulator ops against freed PI data during unregister

This clears pcdev->pi after freeing it in pse_release_pis() and adds a
NULL check to pse_pi_is_enabled(), pse_pi_enable() and pse_pi_disable(),
with the kfree()/NULL store and the reads both done under pcdev->lock.
The stated goal is to avoid a use-after-free when the regulator core
calls into these ops after the controller has been unregistered.

> 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;
> +	mutex_unlock(&pcdev->lock);
>  }

[Severity: High]
Now that pcdev->pi can legitimately be NULL, do the other readers of
pcdev->pi in this file need the same guard?  Only the three regulator ops
were updated, but the consumer-facing paths still index the array
unconditionally:

drivers/net/pse-pd/pse_core.c:__pse_control_release() {
	...
	if (psec->pcdev->pi[psec->id].admin_state_enabled)
		regulator_disable(psec->ps);
	...
}

The same unguarded read appears in pse_get_sw_admin_state(),
pse_ethtool_get_status(), pse_ethtool_set_config() (four times),
pse_pi_update_pw_budget(), pse_ethtool_set_pw_limit() and
pse_ethtool_set_prio().

Can these run after pse_release_pis()?  pse_controller_unregister() does
not drain pcdev->pse_control_head, and a pse_control only takes
try_module_get(pcdev->owner) in pse_control_get_internal(), which blocks
module unload but not driver unbind.  The handle lives in phydev->psec and
is dropped only by phy_device_remove() -> pse_control_put().  So after
unbinding the PSE controller driver, an ethtool PSE request on the still
registered interface, or a later PHY removal, would reach these reads with
pcdev->pi == NULL.

Before this change those reads were a use-after-free of the freed array;
with the NULL store they become a NULL + offset dereference, so the oops
becomes deterministic rather than going away.

Two of the lock-holding readers also sample the pointer before acquiring
pcdev->lock, so the lock-mediated NULL store cannot be observed by them:

drivers/net/pse-pd/pse_core.c:pse_ethtool_get_status() {
	...
	pi = &pcdev->pi[psec->id];
	mutex_lock(&pcdev->lock);
	if (pi->pw_d) {
	...
}

pse_pi_update_pw_budget() does the same with
struct pse_pi *pi = &pcdev->pi[id];

Does the invariant described in the commit message ("read pcdev->pi under
the same lock in the ops") hold for those two?

[Severity: High]
This isn't a bug introduced by this patch, but the ordering in
pse_controller_unregister() looks like it leaves part of the window the
commit message claims to close still open:

drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
	...
	cancel_work_sync(&pcdev->ntf_work);
	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	...
}

pse_flush_pw_ds() drops the power-domain kref, and __pse_pw_d_release()
calls regulator_put(pw_d->supply) plus xa_erase() without clearing the
pcdev->pi[i].pw_d back-pointers and without taking pcdev->lock.

In the interval between pse_flush_pw_ds() and the kfree()/NULL store,
pcdev->pi is still non-NULL, so the new guards pass and the ops proceed to
the released supply:

pse_pi_disable() -> _pse_pi_disable() -> pse_pi_deallocate_pw_budget() {
	regulator_free_power_budget(pi->pw_d->supply, pi->pw_allocated_mW);
}

and via pse_pw_d_retry_power_delivery() ->
pse_pi_allocate_pw_budget_static_prio() ->
regulator_request_power_budget(pi->pw_d->supply, ...).

Both helpers write rdev->pw_requested_mW, so this would be a
use-after-free write through a freed struct regulator.  Does the stated
guarantee ("the op either sees the live array or returns without touching
freed memory") cover the case where the array is live but the supply it
points at is already released?

[ ... ]

> @@ -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;
> +	}

[Severity: Medium]
Should this path return success without ever de-energizing the port?
Taking the early exit means ops->pi_disable() is never called, so the PSE
chip is not told to stop sourcing power, and pse_pi_deallocate_pw_budget()
is skipped, so the mW reserved on the shared power-domain supply are not
returned via regulator_free_power_budget().

The reachable trigger looks like a race rather than the deferred-disable
flush named in the comment: pse_ethtool_set_config() reads
pcdev->pi[psec->id].admin_state_enabled while the array is live,
pse_release_pis() then frees and NULLs it on the detaching CPU, and the
consumer's regulator_disable() lands here and returns 0.  Before this
change the same sequence still issued the ops->pi_disable() register write.

pcdev->ops and the driver private state are released later in devres LIFO
order, and id comes from rdev_get_id(rdev), so is there a reason the
hardware disable cannot still be issued here?

Also, pse_pi_enable() returns -ENODEV for the same condition while this one
returns 0.  Is the asymmetry intended?

>  	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;

[Severity: High]
This isn't a bug introduced by this patch, but since these lines are being
reworked anyway: should ret be returned here instead of 0?

_pse_pi_disable() returns -EOPNOTSUPP when ops->pi_disable is absent, and
otherwise propagates the driver error from the port-off register write
(for example an I2C failure in tps23881_pi_disable() or
pd692x0_pi_disable()).

With 0 returned, _regulator_do_disable() treats the disable as successful,
_regulator_disable() sets rdev->use_count = 0 and the consumer's
enable_count is decremented, while the hardware is still sourcing power
and pcdev->pi[id].admin_state_enabled stays at 1 because the assignment is
skipped on exactly that error path.

The ethtool "admin-state disabled" request then reports success with the
port still energized, pse_ethtool_set_config() will not re-enable it since
if (!admin_state_enabled) is false, and a further disable attempt calls
regulator_disable() with use_count == 0, which the regulator core rejects
with a WARN and -EIO.

[Severity: High]
One more pre-existing item, not introduced by this patch, in
pse_controller_register() in the same file: there is no error unwinding
after the early allocations.

After kfifo_alloc(&pcdev->ntf_fifo, ...) succeeds, the mandatory callback
check returns without kfifo_free():

drivers/net/pse-pd/pse_core.c:pse_controller_register() {
	...
	if (!pcdev->ops->pi_get_admin_state ||
	    !pcdev->ops->pi_get_pw_status) {
		dev_err(...);
		return -EINVAL;
	}
	...
}

Once of_load_pse_pis() has allocated pcdev->pi and taken OF node
references, the later failures (ops->setup_pi_matrix(), the devm_kzalloc()
of the regulator name, devm_pse_pi_regulator_register(),
pse_register_pw_ds()) also return directly without pse_release_pis(), so
the non-devm PI array and the of_node_get() references leak.

Can a failure inside pse_register_pw_ds() leave a dangling global entry?
An earlier iteration may already have inserted a devm_kzalloc()-backed
pse_power_domain into pse_pw_d_map via devm_pse_alloc_pw_d() -> xa_alloc()
and taken a regulator_get() reference.  devm_pse_controller_register() only
does devres_free(pcdevp) on failure, so nothing erases that entry; devres
then frees the pw_d while the xarray still points at it, and the next
registration walks xa_for_each(&pse_pw_d_map, index, pw_d) and calls
regulator_is_equal(pw_d->supply, ...) on freed memory.
-- 
pw-bot: cr

[PATCH net v3 3/3] net: pse-pd: unregister from the controller list before freeing PI data

[Carlo Szelinsky]

pse_controller_unregister() frees the PI array with pse_release_pis()
while the controller is still linked on pse_controller_list, and only
removes it from the list afterwards. A concurrent consumer probe running
of_pse_control_get() walks that list under pse_list_mutex and calls
of_pse_match_pi(), which dereferences pcdev->pi[i].np. If the walk lands
on a controller that is being torn down, it reads the freed (with the
previous patch, NULLed) PI array.

Move the list_del() ahead of pse_release_pis(). Both the lookup and the
removal serialise on pse_list_mutex, so once the controller is unlinked
no new lookup can reach it, and any lookup already in progress holds the
mutex and has matched against a live pi before the free can run. No NULL
checks are needed on the lookup path.

Fixes: 9be9567a7c59 ("net: pse-pd: Add support for PSE PIs")
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 21ccb5146616..2a9a07dc8c48 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -1143,11 +1143,15 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 		disable_irq(pcdev->irq);
 	cancel_work_sync(&pcdev->ntf_work);
 	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
-	kfifo_free(&pcdev->ntf_fifo);
+	/* Unlink before freeing pcdev->pi: of_pse_control_get() walks the
+	 * list under pse_list_mutex and dereferences pcdev->pi[] via
+	 * of_pse_match_pi(), so a lookup must never reach a freed array.
+	 */
 	mutex_lock(&pse_list_mutex);
 	list_del(&pcdev->list);
 	mutex_unlock(&pse_list_mutex);
+	pse_release_pis(pcdev);
+	kfifo_free(&pcdev->ntf_fifo);
 }
 EXPORT_SYMBOL_GPL(pse_controller_unregister);
 
-- 
2.43.0

Re: [PATCH net v3 3/3] net: pse-pd: unregister from the controller list before freeing PI data

[Kory Maincent]

On 8/13/26 22:06, Carlo Szelinsky wrote:
> pse_controller_unregister() frees the PI array with pse_release_pis()
> while the controller is still linked on pse_controller_list, and only
> removes it from the list afterwards. A concurrent consumer probe running
> of_pse_control_get() walks that list under pse_list_mutex and calls
> of_pse_match_pi(), which dereferences pcdev->pi[i].np. If the walk lands
> on a controller that is being torn down, it reads the freed (with the
> previous patch, NULLed) PI array.
> 
> Move the list_del() ahead of pse_release_pis(). Both the lookup and the
> removal serialise on pse_list_mutex, so once the controller is unlinked
> no new lookup can reach it, and any lookup already in progress holds the
> mutex and has matched against a live pi before the free can run. No NULL
> checks are needed on the lookup path.
> 
> Fixes: 9be9567a7c59 ("net: pse-pd: Add support for PSE PIs")
> Signed-off-by: Carlo Szelinsky <github@szelinsky.de>

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com