Re: [PATCH net v2 1/2] net: pse-pd: stop async event sources before freeing PI data in unregister

[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.
https://sashiko.dev/#/patchset/20260711121611.1639086-2-github@szelinsky.de?part=1
---
--- Patch [1]: [PATCH net v2 1/2] net: pse-pd: stop async event sources before freeing PI data in unregister ---
commit bc4c653b9b2b42b2c22ca8b6cae3af394c8add0e
Author: Carlo Szelinsky <github@szelinsky.de>

net: pse-pd: stop async event sources before freeing PI data in unregister

This commit reorders the teardown process in pse_controller_unregister().
It disables the IRQ and flushes the notification worker before flushing power
domains and releasing the PI array. This is intended to prevent use-after-free
issues caused by asynchronous accessors referencing freed data structures.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index a5e6d7b26b9fd..6045b6c399c26 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);

[Severity: High]
Does this code call disable_irq() on an already freed IRQ line?

In tps23881.c, devm_pse_controller_register() is called before
tps23881_setup_irq(). Because devres executes teardown actions in LIFO order,
the IRQ is freed before pse_controller_unregister() runs.

Calling disable_irq() here operates on a freed IRQ descriptor, which can cause
warnings or corrupt state.

>  	cancel_work_sync(&pcdev->ntf_work);
> +	pse_flush_pw_ds(pcdev);
> +	pse_release_pis(pcdev);

[Severity: High]
This isn't a bug introduced by this patch, but releasing the PIs here causes a
use-after-free via sysfs.

In pse_controller_register(), PI regulators are registered via
devm_pse_pi_regulator_register() before devm_pse_controller_register() is called
by the driver. Devres LIFO ordering means pse_controller_unregister() runs
first, freeing pcdev->pi.

The regulators remain active until their devres actions run later.

If a user space read via sysfs triggers pse_pi_is_enabled() during this window,
it will dereference the freed pcdev->pi array.

[Severity: High]
This is a pre-existing issue, but does freeing the power domains here create a
use-after-free for shared domains?

In pse_register_pw_ds(), shared power domains are added to a global pse_pw_d_map
and their refcount is incremented, but they are allocated with devm_kzalloc()
tied to a specific device.

When the original allocating device is unbound, devres frees the memory, leaving
dangling pointers in the global map and in any other device sharing the domain.

>  	kfifo_free(&pcdev->ntf_fifo);
>  	mutex_lock(&pse_list_mutex);
>  	list_del(&pcdev->list);

[Severity: High]
This is a pre-existing issue, but does destroying internal data structures
before removing the controller from the global list create a use-after-free?

Because pse_release_pis() runs before list_del(), a concurrent device probe
calling of_pse_control_get() can iterate over pse_controller_list, call
of_pse_match_pi(), and dereference the already-freed pcdev->pi[i].np.

[Severity: High]
This is a pre-existing issue, but the PSE control references don't seem to
manage object lifetime properly.

When a consumer gets a control via of_pse_control_get(), it takes a module
reference but does not establish a device link enforcing that the supplier
outlives the consumer.

If the PSE controller is unbound via sysfs, this teardown path frees pcdev->pi.
The active consumer can then dereference freed memory when it later calls
__pse_control_release().

[Severity: Medium]
This is a pre-existing issue, but does pse_controller_register() leak memory on
error paths?

If setup steps fail after allocating the kfifo and the PI structures (e.g.,
missing callbacks or setup_pi_matrix() failures), the function returns the
error without calling kfifo_free() or pse_release_pis().