Re: [PATCH net 2/2] net: pse-pd: guard against freed PI data on regulator disable

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: net: pse-pd: guard against freed PI data on regulator disable

This patch closes a use-after-free reachable from the regulator core's
deferred-disable flush during driver detach by clearing pcdev->pi after
freeing it in pse_release_pis() and bailing out in pse_pi_disable() when
pcdev->pi is NULL.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 1f27884dd4d7..752d987a7004 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -145,6 +145,7 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)
>  		of_node_put(pcdev->pi[i].np);
>  	}
>  	kfree(pcdev->pi);
> +	pcdev->pi = NULL;
>  }

[High]
Is the pcdev->pi = NULL store correctly synchronized with the readers?

The reader side in pse_pi_disable() observes pcdev->pi under
pcdev->lock:

    mutex_lock(&pcdev->lock);
    if (!pcdev->pi) {
            mutex_unlock(&pcdev->lock);
            return 0;
    }

But pse_release_pis() does the kfree() and the NULL store without
acquiring pcdev->lock, and pse_controller_unregister() does not wrap
the pse_release_pis() call with that lock either:

    void pse_controller_unregister(struct pse_controller_dev *pcdev)
    {
            pse_flush_pw_ds(pcdev);
            if (pcdev->irq)
                    disable_irq(pcdev->irq);
            pse_release_pis(pcdev);
            ...
    }

In the LIFO devres scenario described in the commit message, the later
regulator_unregister() does flush_work() on the deferred disable, which
provides the necessary memory barrier, so that specific case is fixed.

However, regulator consumers in other drivers (PHY/MAC) hold a
struct regulator * to a PSE PI and can call regulator_disable() (which
may take the pse_pi_disable() path directly) on a different CPU,
concurrent with the unbind path running pse_release_pis() on this
device.

That concurrent reader can take pcdev->lock and observe pcdev->pi as
still non-NULL while pse_release_pis() is concurrently kfree()-ing the
array, leading to a UAF in the subsequent _pse_pi_disable(pcdev, id)
and pcdev->pi[id].admin_state_enabled = 0 dereferences.

Would taking pcdev->lock around the kfree()+NULL store in
pse_release_pis() (so the NULL observation under pcdev->lock is
authoritative) close that window? Alternatively, can the regulator
unregister be ordered to run before pse_release_pis() so no consumer
can invoke a regulator op while the PI array is being torn down?

> @@ -702,15 +703,21 @@ 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);
> +	/* The controller may already be unregistered (pcdev->pi freed) by the
> +	 * time the regulator core flushes a deferred disable during
> +	 * regulator_unregister(). Bail out to avoid touching freed PI data.
> +	 */
> +	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;

[High]
Should the same NULL guard be applied to pse_pi_is_enabled() and
pse_pi_enable()?

Both callbacks follow the identical pattern as the disable path:
rdev_get_drvdata(rdev) -> mutex_lock(&pcdev->lock) -> unconditional
pcdev->pi[id].* access.

In pse_pi_is_enabled():

    id = rdev_get_id(rdev);
    mutex_lock(&pcdev->lock);
    if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
            ret = pcdev->pi[id].admin_state_enabled;
            ...

And in pse_pi_enable():

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

The lifetime invariant cited in the commit message ("the controller
may already be unregistered by the time the regulator core flushes...")
is a property of the LIFO devres unwind order in which
pse_controller_unregister() runs before the devm-registered regulators
are torn down.

While regulator_unregister() only flushes disable_work, regulator
consumers in other drivers that hold a struct regulator * to a PSE PI
can still call regulator_is_enabled() or regulator_enable() concurrent
with the in-progress pse_controller_unregister() on the parent device.
At that point pcdev->pi may already be NULL/freed and the unguarded
pcdev->pi[id].pw_d / pcdev->pi[id].admin_state_enabled dereferences
would be a UAF or NULL deref.

Should these two callbacks receive the same guard, or alternatively
should the unwind order be changed so that no callback can fire after
the PI array is freed?