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@xxxxxxxxxxxx?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@xxxxxxxxxxxx>

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;