Re: [PATCH] clk: samsung: exynos5-subcmu: Defer probe until power domains are available

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] clk: samsung: UAF panic via -EPROBE_DEFER in __init probe function
- [High] clk: samsung: async pm_runtime_get in subcmu probe causes unpowered reg reads
- [Medium] clk: samsung: sub-CMU registered without PM domain for disabled nodes
- [Medium] clk: samsung: missing of_genpd_add_device() error check
--

commit b224f204096c564d4ab18ceac4426ac6bc78aaed
Author: Marek Szyprowski <m.szyprowski@samsung.com>

clk: samsung: exynos5-subcmu: Defer probe until power domains are available
    
Sub-CMU devices are added to their power domains with of_genpd_add_device()
during exynos5_clk_probe(). Its return value is ignored, so if the power
domain provider is not registered yet, the sub-CMU device is silently left
outside of its power domain.

Since commit c09a3e6c97f0 ("soc: samsung: pm_domains: Convert to regular
platform driver"), power domain providers are registered only when their
platform devices are probed, which happens after exynos5_clk_probe() has
already been called.

When a sub-CMU device is outside of its power domain, its runtime PM
callbacks race with the power domain state changes, which might result in
losing the proper hardware state. In practice, some clock muxes are left
switched to the oscillator instead of the respective PLLs.

Fix this by deferring exynos5_clk_probe() until the platform devices for
all available power domains are instantiated.

> diff --git a/drivers/clk/samsung/clk-exynos5-subcmu.c b/drivers/clk/samsung/clk-exynos5-subcmu.c
> index 3731298473013..587fe6e3bbed2 100644
> --- a/drivers/clk/samsung/clk-exynos5-subcmu.c
> +++ b/drivers/clk/samsung/clk-exynos5-subcmu.c

[ ... ]

> @@ -155,11 +156,25 @@ static int __init exynos5_clk_register_subcmu(struct device *parent,
>  
>  static int __init exynos5_clk_probe(struct platform_device *pdev)
>  {
> +	static const char pd_node_name[] = "samsung,exynos4210-pd";
>  	struct device_node *np;
>  	const char *name;
>  	int i;
>  
> -	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
> +	/* Defer until all needed power domains are registered */
> +	for_each_compatible_node(np, NULL, pd_node_name) {
> +		if (of_device_is_available(np)) {
> +			struct platform_device *pd = of_find_device_by_node(np);
> +
> +			if (!pd) {
> +				of_node_put(np);
> +				return -EPROBE_DEFER;

[Severity: Critical]
Since exynos5_clk_probe() is marked __init, will returning -EPROBE_DEFER
cause a use-after-free panic?

When probe deferral is triggered, the probe function is queued for retry.
Because the function resides in the __init section, its memory is freed
after boot. When the deferred probe workqueue eventually retries probing
the device, the kernel will jump into freed memory.

(The modpost section mismatch warning that would normally prevent assigning
an __init function to a platform driver's probe callback is masked by the
__refdata annotation on exynos5_clk_driver).

> +			}
> +			put_device(&pd->dev);
> +		}
> +	}
> +
> +	for_each_compatible_node(np, NULL, pd_node_name) {
>  		if (of_property_read_string(np, "label", &name) < 0)
>  			continue;
>  		for (i = 0; i < nr_cmus; i++)
>  			if (strcmp(cmu[i]->pd_name, name) == 0)
>  				exynos5_clk_register_subcmu(&pdev->dev,
>  							    cmu[i], np);

[Severity: Medium]
Since this second loop doesn't have the of_device_is_available() check
used in the first loop, could this instantiate sub-CMU devices for disabled
PM domains?

For disabled nodes, this will extract the label, find a matching sub-CMU,
and call exynos5_clk_register_subcmu(). Because the PM domain provider for
the disabled node is never registered, of_genpd_add_device() will fail.

[Severity: Medium]
Will masking the return value of exynos5_clk_register_subcmu() leave the
hardware in an invalid state?

If of_genpd_add_device() or platform_device_alloc() fails in
exynos5_clk_register_subcmu(), the error is ignored. The device is registered
without its power domain attached, which can cause runtime PM callbacks to
race with domain state changes, leaving clock muxes in an incorrect state.

>  	}
>  	return 0;
>  }

I also noticed another issue with runtime PM in this driver:

drivers/clk/samsung/clk-exynos5-subcmu.c:exynos5_subcmu_probe() {
    ...
    pm_runtime_set_suspended(dev);
    pm_runtime_enable(dev);
    pm_runtime_get(dev);

    ctx->dev = dev;
    samsung_clk_register_div(ctx, info->div_clks, info->nr_div_clks);
    ...
}

[Severity: High]
Does this asynchronous pm_runtime_get() cause unpowered register reads?

It queues the device power-on but does not wait, and then the immediately
following clock registration functions access the hardware registers. The
Common Clock Framework initialization synchronously reads the clock registers
via .recalc_rate ops (e.g., clk_divider_recalc_rate()).

Because the device resume and genpd power-on are queued asynchronously,
these register reads occur while the power domain is physically off. This
can cause an imprecise external abort (bus fault) or return zeros, leading
to a corrupted clock tree state.

Should this use pm_runtime_get_sync() to ensure the hardware is ready before
access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013111840.3645724-1-m.szyprowski@samsung.com?part=1