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

[netdev-bot+sashiko@kernel.org]

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

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

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] pse_flush_pw_ds() runs after pse_register_pw_ds() has already
  dropped pse_pw_d_mutex, so the new flush_pw_ds unwind can race with…
- [Medium] The new flush_pw_ds unwind in pse_controller_register()
  releases the power domains while the PI regulators are still…
- [Medium] Failures in the PI-regulator loop (devm_kzalloc of reg_name,
  devm_pse_pi_regulator_register()) and in pse_register_pw_ds() still…
- [Low] The patch fixes a use-after-free and several leaks that are in
  released kernels, but it has no Fixes: tag, no stable Cc, and it goes…
- [Low] The commit message says the PI array is released on
  'setup_pi_matrix() and the supply check', but at this commit…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  pse_controller_register() calls kfifo_alloc(&pcdev->ntf_fifo,…
- [High] This problem wasn't introduced by this patch, but
  pse_controller_unregister() frees pcdev->pi through pse_release_pis()…

--

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

This patch adds error unwinding to pse_controller_register(). A failed
registration now frees the notification kfifo and flushes any partially
created power domains. The PI array is released only on failures that
happen before the first PI regulator is registered.

> A partial pse_register_pw_ds() is worse than a leak. The power domains it
> already created are devm-allocated but live in the global pse_pw_d_map,
> so the failed probe frees them while that xarray still points at them,
> and the next controller to register walks into freed memory in
> regulator_is_equal().

[Severity: Low]
This fixes a use-after-free and several leaks that exist in released
kernels. However, the patch has no Fixes: tags and no stable Cc, and it
goes to net-next inside a feature series. Should it carry tags such as:

Fixes: 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")
Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Fixes: 29e28d1d7a16 ("net: pse-pd: Add support for setup_pi_matrix callback")
Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")

The first covers the pse_register_pw_ds() use-after-free, the second the
kfifo leak, and the last two the PI array and OF reference leaks. The
unwind depends on the rest of the series only through a
blocking_notifier_call_chain() context line.

> reach those ops. So the array is released on the failures that happen
> while it exists and before the first PI regulator does: setup_pi_matrix()
> and the supply check.

[Severity: Low]
At this commit, setup_pi_matrix() is the only failure that jumps to
release_pis. pse_controller_register() has no supply check yet. The
supply check that unwinds to release_pis comes in the next patch:

"net: pse-pd: check the PI vpwr supply before registering the controller"

Could the message for this patch describe only the code it contains?

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 56cecf60c5c4c..16d75b4babf34 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1092,17 +1092,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 isn't a bug introduced by this patch, but just above this hunk the
kfifo is sized before nr_lines is normalized:

pse_controller_register() {
    ...
	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);
	...
	if (!pcdev->nr_lines)
		pcdev->nr_lines = 1;
    ...
}

pse_reg_probe() in drivers/net/pse-pd/pse_regulator.c never sets
nr_lines, so kfifo_alloc() is passed 0. __kfifo_alloc_node() then calls
roundup_pow_of_two(0), which evaluates 1UL << BITS_PER_LONG. That shift is
undefined, and either possible result fails the size < 2 check, so
kfifo_alloc() returns -EINVAL.

Does this mean the podl-pse-regulator controller can never finish
pse_controller_register()?

Moving the normalization above kfifo_alloc() would not be enough on its
own, because a capacity of 1 is also rejected. The fifo size would need
to be at least 2.

[ ... ]

> @@ -1119,20 +1125,22 @@ 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]
Do these failure paths leak the OF node references taken in
of_load_pse_pis()?

Three paths end at free_kfifo: the reg_name allocation failure, the
devm_pse_pi_regulator_register() failure and the flush_pw_ds path. On
all of them, the of_node_get() on each PI node and the of_parse_phandle()
reference on each pairset are never dropped.

devm_pse_controller_register() frees only its devres cookie, so these
references leak on every failed probe. The commit message lists "an OF
reference per described PI" among the leaks it fixes.

The reason given for keeping the array does not seem to cover these
references:

- none of the pse_pi_ops touch pi[].np or pairset[].np
- the regulator core takes its own reference with
  of_node_get(config->of_node)
- of_pse_match_pi() is reachable only through pse_controller_list, which
  a failed controller never joins

Could the node references be put, and their pointers cleared, on these
paths without freeing the array?

Also, as the commit message notes, a reg_name failure in the first
iteration happens before any PI regulator exists. Releasing the whole
array would still be safe there.

>  
>  	ret = pse_register_pw_ds(pcdev);
>  	if (ret)
> -		return ret;
> +		goto flush_pw_ds;

[Severity: High]
Can the flush_pw_ds unwind race with another controller that registers
on the same supply?

pse_register_pw_ds() drops pse_pw_d_mutex at its out: label while the
domains it already created are still published in pse_pw_d_map.
pse_flush_pw_ds() runs only afterwards, outside that lock:

CPU1 (controller A)                  CPU2 (controller B, same supply)
pse_register_pw_ds()
  devm_pse_alloc_pw_d() for PI 0
    /* published in pse_pw_d_map */
  later iteration fails
  out: mutex_unlock(&pse_pw_d_mutex)
                                     pse_register_pw_ds()
                                       regulator_is_equal() matches A's pw_d
                                       pcdev->pi[i].pw_d = pw_d;
                                       kref_get(&pw_d->refcnt);
flush_pw_ds:
  pse_flush_pw_ds()
    kref_put_mutex()  /* 2 -> 1 */
probe fails, devres frees pw_d

The refcount only drops from 2 to 1, so __pse_pw_d_release() never runs
and there is no xa_erase() or regulator_put(). The pw_d lifetime comes
from devres on A's device, not from the kref. After the failed probe,
both pse_pw_d_map and B's pi[i].pw_d point at freed memory.

Would the next registration then still read freed memory in
regulator_is_equal()? B's pse_pw_d_is_sw_pw_control() and
pse_pi_allocate_pw_budget*() would also read the freed domain. That looks
like the same use-after-free the commit message describes.

Does this need the unwind to happen inside the mutex-held section of
pse_register_pw_ds()? The alternative would be to stop devm-allocating
the shared, refcounted domain.

[ ... ]

> @@ -1142,6 +1150,19 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  				     PSE_REGISTERED, pcdev);
>  
>  	return 0;
> +
> +flush_pw_ds:
> +	pse_flush_pw_ds(pcdev);
> +	goto free_kfifo;

[Severity: Medium]
Does this leave pcdev->pi[i].pw_d dangling while the PI regulators are
still live?

pse_flush_pw_ds() calls __pse_pw_d_release(), which does
regulator_put(pw_d->supply) and xa_erase(). pcdev->pi[i].pw_d is not
cleared.

The pw_d itself was devm_kzalloc()ed after the PI regulators were
devm-registered. When the failed probe unwinds, devres therefore frees it
before it unregisters the regulators.

In that window, the regulator state attribute or regulator_late_cleanup()
can reach:

pse_pi_is_enabled()
    pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)
        pw_d->budget_eval_strategy   /* freed */

pse_pi_deallocate_pw_budget() and pse_pi_allocate_pw_budget_static_prio()
would also still use the already released pw_d->supply.

The devres ordering window existed before this patch. What is new is that
the flush drops the supply while pi[].pw_d still points at it.

The comment added above the regulator loop says the live regulators must
not be handed a freed pointer. Would setting pcdev->pi[i].pw_d = NULL
after pse_flush_pw_ds() on this path cover it? The ops already treat a
NULL pw_d as no domain.

> +
> +release_pis:
> +	pse_release_pis(pcdev);
> +	pcdev->pi = NULL;

[Severity: High]
This is a pre-existing issue, and the commit message mentions it, but
pse_controller_unregister() still ends with:

	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	kfifo_free(&pcdev->ntf_fifo);

devm_pse_controller_register() adds its devres node after the PI
regulators and pw_d allocations. On unbind, devres therefore calls
pse_controller_unregister() first, and pse_release_pis() frees pcdev->pi
while every PI regulator is still registered:

devm_pse_controller_release()
    pse_controller_unregister()
        pse_release_pis()
            kfree(pcdev->pi)

Can a read of /sys/class/regulator/regulator.N/state (mode 0444) that
races with the unbind then reach this, before devres unregisters the
regulators?

pse_pi_is_enabled()
    pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)

That would dereference a pw_d read from the freed array. The commit
message defers the fix to a pending net teardown patch, and the problem
is still present at the end of this series.

[ ... ]

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