Re: [PATCH net 1/2] net: pse-pd: disable IRQ before freeing PI data in unregister

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

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
---
net: pse-pd: disable IRQ before freeing PI data in unregister

pse_controller_unregister() frees the PI array via pse_release_pis()
before disabling the controller IRQ. The threaded IRQ handler pse_isr()
walks pcdev->pi[] (via pse_set_config_isr() and
regulator_notifier_call_chain() on pcdev->pi[i].rdev), so an interrupt
arriving in the window between pse_release_pis() and disable_irq()
dereferences freed memory.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 69dbdbde9d71a..1f27884dd4d70 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1115,9 +1115,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
>  void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  {
>  	pse_flush_pw_ds(pcdev);
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);

Simon says: The issue flagged below seems to warrant investigation
            within the context of this patch.

[Severity: High]
This is a pre-existing issue, but does leaving pse_flush_pw_ds() before
disable_irq() risk a use-after-free of a regulator during an interrupt?

pse_flush_pw_ds() drops references to power domains, which can trigger
__pse_pw_d_release() and free the regulator via regulator_put(pw_d->supply).

If an interrupt arrives after this but before the IRQ is disabled:

pse_isr()
  _pse_pi_disable()
    pse_pw_d_retry_power_delivery()
      regulator_request_power_budget(pw_d->supply, ...)

Could this access the freed regulator? Should disable_irq() be the very
first operation in the teardown sequence?

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

Simon says: Although this is a pre-exiting issue, it does seem to
            warrant investigation within the context of this patch-set.

[Severity: High]
This isn't a bug introduced by this patch, but can calling
cancel_work_sync() after pse_release_pis() cause an indirect
use-after-free of pcdev->pi?

The worker blocks during cancel_work_sync(), and it can call:

pse_send_ntf_worker()
  pse_control_put(psec)
    __pse_control_release()

Since __pse_control_release() explicitly dereferences
psec->pcdev->pi[psec->id].admin_state_enabled, can the worker execute this
release path while or after pcdev->pi is freed by pse_release_pis()?

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

Simon says: I believe the remaining issues should be investigated
            in the context of possible follow-up or separate bug
	    fixes.

[Severity: High]
This is a pre-existing issue, but does removing the device from the global
list at the very end of unregister allow concurrent lookups to access
freed memory?

The device remains discoverable via of_pse_control_get() while its
resources are actively being destroyed. If a concurrent thread calls
of_pse_control_get(), it will find the pcdev and call
pse_control_get_internal(), which accesses pcdev->pi[index].

Because pse_release_pis() has already freed pcdev->pi earlier in this
sequence, could this cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but does the unregistration sequence lack
synchronization with external pse_control consumers?

of_pse_control_get() provides a pse_control struct to consumers (like PHY
drivers), using try_module_get() but without incrementing the device
reference count or creating strict device links.

When a PSE controller is unbound, pse_controller_unregister() synchronously
frees pcdev->pi. However, external clients may still hold struct
pse_control references.

When these clients are later removed, they call:

pse_control_put(psec)
  __pse_control_release()
    dereferences psec->pcdev->pi[psec->id]

Since pcdev->pi was already freed, does this cause a use-after-free on
client teardown?

[Severity: High]
This is a pre-existing issue, but does the devm teardown sequence free
internal structures while the PSE PI regulators remain fully accessible?

pse_controller_register() registers regulators using
devm_regulator_register(). Afterwards, devm_pse_controller_register()
registers its own release callback.

During driver unbind, the LIFO devm sequence means
devm_pse_controller_release() runs first, freeing pcdev->pi via
pse_release_pis().

However, the regulators are still active and visible in sysfs until the
subsequent devm teardown phase unregisters them. A concurrent sysfs access
(e.g., reading regulator state) during this window will invoke regulator
operations like pse_pi_is_enabled(), which dereference the already freed
pcdev->pi[id].

Could this result in a use-after-free?

[Severity: High]
This is a pre-existing issue, but is the globally shared and
reference-counted struct pse_power_domain object incorrectly tied to the
lifetime of the device that allocated it?

devm_pse_alloc_pw_d() allocates pw_d using devm_kzalloc(). The memory
lifetime is therefore bound to the registering PSE controller. This pw_d is
added to the global pse_pw_d_map and uses kref for reference counting.

pse_register_pw_ds() allows other PSE controllers to discover and share
this pw_d by incrementing its kref.

If the device that originally created pw_d unbinds, devm will unilaterally
free the pw_d memory, ignoring the kref counts held by other devices. Any
subsequent access by those other devices will result in a use-after-free.

The __pse_pw_d_release() function seems to omit kfree(pw_d) because it
relies on devm, but doesn't this break the kref requirement?