Re: [PATCH v3 05/12] pmdomain: Make genpd_get_from_provider() public

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Exporting of_genpd_get_from_provider introduces an unsafe public API that returns a bare pointer to a generic_pm_domain without reference counting or external locking, enabling Use-After-Free vulnerabilities.
--

commit 3c8789e2e8b48c33d816b4234b9862d8dcb170e7
Author: Geert Uytterhoeven <geert+renesas@glider.be>

pmdomain: Make genpd_get_from_provider() public

This commit renames genpd_get_from_provider() to of_genpd_get_from_provider()
and exports it to allow PM domain drivers to look up providers directly.

> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 842c4169e2906..ac3a07066e7fc 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
[ ... ]
> @@ -2900,8 +2900,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>   * Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
>   * on failure.
>   */
> -static struct generic_pm_domain *genpd_get_from_provider(
> -					const struct of_phandle_args *genpdspec)
> +struct generic_pm_domain *of_genpd_get_from_provider(const struct of_phandle_args *genpdspec)
>  {
>  	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>  	struct of_genpd_provider *provider;
> @@ -2923,6 +2922,7 @@ static struct generic_pm_domain *genpd_get_from_provider(
>  
>  	return genpd;
>  }
> +EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);

[Severity: High]
Could exporting this function introduce a Use-After-Free vulnerability for
callers?

Looking at the implementation of of_genpd_get_from_provider(), it searches
for the provider while holding of_genpd_mutex, but drops the lock before
returning the bare generic_pm_domain pointer. Since generic_pm_domain does
not appear to use a kref or external synchronization mechanism, the returned
pointer is left completely unprotected once the lock is dropped.

If the provider is concurrently unregistered (for example, via unbinding the
device from sysfs), is it possible for pm_genpd_remove() to free the domain
memory while an external caller is still holding or mutating this stale
pointer?

[ ... ]
> @@ -2995,7 +2995,7 @@ int of_genpd_add_child_ids(struct device_node *np,
>  			goto err_unwind;
>  
>  		/* Get the parent domain */
> -		parent_genpd = genpd_get_from_provider(&parent_args);
> +		parent_genpd = of_genpd_get_from_provider(&parent_args);
>  		of_node_put(parent_args.np);
>  		if (IS_ERR(parent_genpd)) {
>  			pr_err("Failed to get parent domain for %pOF: %ld\n",

[Severity: High]
Does this sequence present a Time-Of-Check to Time-Of-Use (TOCTOU) race?

Because of_genpd_get_from_provider() drops the lock before returning, there
is a window between retrieving parent_genpd and subsequently using it (such
as passing it to pm_genpd_add_subdomain() later in this function).

Could the parent domain be freed by a concurrent unregistration during this
window, before the necessary locks are acquired by the subsequent operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788338320.git.geert+renesas@glider.be?part=5