Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The use-after-free the patch says it closes is still reachable
  when two controllers register concurrently.
- [Medium] The subject says the patch unwinds allocations when
  registration fails.
- [Medium] pse_flush_pw_ds() now stores NULL to pcdev->pi[i].pw_d (lines
  941 and 954) so that nothing is left pointing at the devm-allocated…
- [Low] The patch fixes a real memory-safety bug but carries no Fixes:
  tag and no stable Cc.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the podl-pse-
  regulator driver can never register a PSE controller.

--

net: pse-pd: unwind allocations when controller registration fails

This patch adds error unwinding to pse_controller_register(). The
notification kfifo is now freed on failure, and early failures also free
the PI array and its OF references. Power domains left by a partial
pse_register_pw_ds() are flushed, pse_flush_pw_ds() now clears pi[].pw_d,
and failures from the PI regulator loop onward deliberately leave the PI
array allocated.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index dc261beb61702..eeefbf25e6716 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -937,11 +937,21 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
>  			continue;
>  
>  		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
> -		if (!pw_d)
> +		if (!pw_d) {
> +			pcdev->pi[i].pw_d = NULL;
>  			continue;
> +		}
>  
>  		kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release,
>  			       &pse_pw_d_mutex);
> +		/* The pw_d is devm memory of whichever controller created
> +		 * it, so it can go away as soon as that probe unwinds.
> +		 * Nothing may be left pointing at it: pse_pi_is_enabled()
> +		 * reaches pi[].pw_d from the regulator "state" attribute,
> +		 * which stays readable until the PI regulators are
> +		 * unregistered after us.
> +		 */
> +		pcdev->pi[i].pw_d = NULL;

[Severity: Medium]
Is this store enough to keep pse_pi_is_enabled() away from a freed pw_d?
The store is done without pcdev->lock, but the reader holds that lock:

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

On the new pse_register_pw_ds() failure path the PI regulators are still
live. That means the world-readable state attribute can reach this at any
time:

state_show()->_regulator_is_enabled()->pse_pi_is_enabled()

Say a reader has loaded pcdev->pi[id].pw_d and is preempted before
pse_pw_d_is_sw_pw_control() reads pw_d->budget_eval_strategy. In that gap,
pse_flush_pw_ds() can clear the field and return, and the probe fails.
devres then frees the pw_d before it unregisters the PI regulators,
because the pw_d was allocated after them. The reader then reads freed
memory.

Would it close this window to take pcdev->lock around the pointer clear
and do the kref_put_mutex() calls afterwards, outside pcdev->lock?

There is a second race in the intermediate state of the series.
pse_ethtool_get_status() checks pi->pw_d, calls the sleeping
pi_get_admin_state(), and then does:

	switch (pi->pw_d->budget_eval_strategy) {

If that races with pse_controller_unregister(), the new unlocked NULL
store turns this into a NULL dereference. Later in the series, "net: phy:
own phydev->psec via PSE notifier and remove fwnode_mdio hook" closes
that part: the ethtool PSE paths take pse_phy_lock(), and psec is
detached before the flush. The pse_pi_is_enabled() window is still there
at the end of the series.

[ ... ]

> @@ -1092,17 +1102,18 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	    !pcdev->ops->pi_get_pw_status) {
>  		dev_err(pcdev->dev,
>  			"Mandatory status report callbacks are missing");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto free_kfifo;
>  	}

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
this patch reworks the kfifo handling here. Can the kfifo ever be
allocated for podl-pse-regulator? Earlier in pse_controller_register():

	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);
	if (ret) {
		dev_err(pcdev->dev, "failed to allocate kfifo notifications\n");
		return ret;
	}
	INIT_WORK(&pcdev->ntf_work, pse_send_ntf_worker);

	if (!pcdev->nr_lines)
		pcdev->nr_lines = 1;

The kfifo size comes from nr_lines before the default is applied.
__kfifo_alloc_node() rounds the size up to a power of two and returns
-EINVAL if the result is below 2, and both 0 and 1 end up there.

pse_reg_probe() in drivers/net/pse-pd/pse_regulator.c never sets
nr_lines before it calls devm_pse_controller_register(). That probe
therefore seems to always fail with "failed to allocate kfifo
notifications". A controller with nr_lines == 1 fails the same way.

Would it be better to size the kfifo from the defaulted nr_lines with a
minimum of 2, for example max(nr_lines, 2)? This came in with
ffef61d6d273 ("net: pse-pd: Add support for budget evaluation
strategies"), and it is still the same at the end of the series.

[ ... ]

> @@ -1110,7 +1121,12 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	 */
>  	reg_name_len = strlen(dev_name(pcdev->dev)) + 18;
>  
> -	/* Register PI regulators */
> +	/* Register PI regulators. Once one of these exists, pse_pi_ops index
> +	 * pcdev->pi[] and nothing here can unregister it again, so the array
> +	 * must outlive this function. Failures below therefore unwind to
> +	 * free_kfifo and deliberately leak it, as they already do today,
> +	 * rather than hand the live regulators a freed pointer.
> +	 */
>  	for (i = 0; i < pcdev->nr_lines; i++) {
>  		char *reg_name;
>  
> @@ -1119,20 +1135,27 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  			continue;
>  
>  		reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL);
> -		if (!reg_name)
> -			return -ENOMEM;
> +		if (!reg_name) {
> +			ret = -ENOMEM;
> +			goto free_kfifo;
> +		}
>  
>  		snprintf(reg_name, reg_name_len, "pse-%s_pi%d",
>  			 dev_name(pcdev->dev), i);
>  
>  		ret = devm_pse_pi_regulator_register(pcdev, reg_name, i);
>  		if (ret)
> -			return ret;
> +			goto free_kfifo;
>  	}

[Severity: Medium]
Doesn't this still leak pcdev->pi and the OF references on the PI and
pairset nodes? That happens on every failure from here on: the reg_name
allocation, this one, and the pse_register_pw_ds() failure below. It even
happens on the first loop iteration, before any PI regulator exists.

On registration failure, devm_pse_controller_register() frees only its
devres cookie, so nothing else releases the array.

The commit message says:

    Freeing the array safely past the loop
    would need the regulator ops to tolerate a NULL pcdev->pi, which they do
    not today.

Is that really the only way to do it? devm_pse_pi_regulator_register()
registers the PI regulators on pcdev->dev with devm_regulator_register().
A devm action on pcdev->dev that releases the PIs could be added right
after of_load_pse_pis() succeeds. Devres releases in LIFO order, so that
action would run after every PI regulator has been unregistered. No
regulator op would ever see a freed or NULL pcdev->pi.

That would also cover pse_controller_unregister(), which the commit
message notes calls pse_release_pis() while the PI regulators are still
registered.

The later patch "net: pse-pd: check the PI vpwr supply before registering
the controller" moves the common -EPROBE_DEFER case ahead of the loop.
The leaks from the loop and from pse_register_pw_ds() are still there at
the end of the series.

>  
>  	ret = pse_register_pw_ds(pcdev);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		/* Deliberately not release_pis: the PI regulators registered
> +		 * above index pcdev->pi[] and outlive this function.
> +		 */
> +		pse_flush_pw_ds(pcdev);
> +		goto free_kfifo;
> +	}

[Severity: High]
Can a controller probing at the same time still hit the use-after-free
the commit message describes? On its error path, pse_register_pw_ds()
drops pse_pw_d_mutex while the domains it just created are still in
pse_pw_d_map:

out:
	mutex_unlock(&pse_pw_d_mutex);
	return ret;

pse_flush_pw_ds() runs only after that, without the mutex. Here is one
possible sequence, where controller B shares the same vpwr supply:

controller A
pse_register_pw_ds()
  devm_pse_alloc_pw_d()       /* pw_d published in pse_pw_d_map */
  regulator_get() fails on a later PI
  mutex_unlock(&pse_pw_d_mutex)

controller B
pse_register_pw_ds()
  regulator_is_equal(pw_d->supply, rdev->supply) matches
  pcdev->pi[i].pw_d = pw_d
  kref_get(&pw_d->refcnt)     /* 1 -> 2 */

controller A
pse_flush_pw_ds()
  kref_put_mutex()            /* 2 -> 1, no xa_erase() */
devres frees pw_d

Now pse_pw_d_map and B's pi[].pw_d both point at freed memory. The next
pse_register_pw_ds() reads pw_d->supply, and B's own flush later does
kref_put on the freed object.

Every domain the failing call created stays private to it until
pse_pw_d_mutex is dropped. Could pse_register_pw_ds() undo those domains
itself before the out: label, doing kref_put/xa_erase/regulator_put while
it still holds pse_pw_d_mutex? That would close this window without
moving pw_d out of devm memory.

[Severity: Low]
Should this patch carry Fixes: tags and a stable Cc? A partial
pse_register_pw_ds() leaves freed power domains in pse_pw_d_map, so this
fixes a real use-after-free. The commit message has only a Signed-off-by.

According to blame, the bare returns this patch replaces came from these
commits:

- after pse_register_pw_ds(): 50f8b341d268 ("net: pse-pd: Add support
  for PSE power domains")
- kfifo_alloc(): ffef61d6d273 ("net: pse-pd: Add support for budget
  evaluation strategies")
- after of_load_pse_pis(): 9be9567a7c59 ("net: pse-pd: Add support for
  PSE PIs")
- after setup_pi_matrix(): 29e28d1d7a16 ("net: pse-pd: Add support for
  setup_pi_matrix callback")

Could this also go to net on its own, rather than sit inside the
notifier chain / phylib ownership series?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004164219.1161294-1-github%40szelinsky.de