Re: [PATCH v3 2/3] clk: qcom: gcc-glymur: Enable runtime PM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The GDSC registration framework initializes power domains using `pm_genpd_init()` but fails to call `pm_genpd_remove()` during unregistration or on probe error paths. This leaves dangling pointers in the global `gpd_list` when the module is unloaded, leading to a Use-After-Free (UAF).
- [High] The error path in `qcom_cc_really_probe()` uses the asynchronous `pm_runtime_put()` before returning an error, triggering devres cleanup which immediately calls `pm_runtime_disable()`. This cancels the pending asynchronous idle/suspend work, permanently leaking the power domain's ON state.
--

--- Patch [2]: [PATCH v3 2/3] clk: qcom: gcc-glymur: Enable runtime PM ---
commit 5dd235050afe17992844a83f5c4a8e644843386b
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

clk: qcom: gcc-glymur: Enable runtime PM

This commit enables runtime PM for the controller so the common GCC probe
path resumes the attached domain while registering clocks, resets and GDSCs.

> diff --git a/drivers/clk/qcom/gcc-glymur.c b/drivers/clk/qcom/gcc-glymur.c
> index f4ede4a3a1c07..1d52150721adc 100644
> --- a/drivers/clk/qcom/gcc-glymur.c
> +++ b/drivers/clk/qcom/gcc-glymur.c
> @@ -8576,6 +8576,7 @@ static const struct qcom_cc_desc gcc_glymur_desc = {
>  	.num_resets = ARRAY_SIZE(gcc_glymur_resets),
>  	.gdscs = gcc_glymur_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gcc_glymur_gdscs),

[Severity: Critical]
This isn't a bug introduced by this patch, but does registering these GDSCs
expose a use-after-free vulnerability during module unload or probe failure?

Looking at gdsc_unregister() in drivers/clk/qcom/gdsc.c:

gdsc_unregister() {
    ...
	gdsc_pm_subdomain_remove(desc, num);
	of_genpd_del_provider(dev->of_node);
}

It appears pm_genpd_remove() is never called to unlink the domains from the
global gpd_list. If gdsc_register() fails halfway or the module is unloaded,
the memory backing the GDSC structs is freed. 

Could any subsequent iteration of gpd_list (e.g., by
genpd_power_off_unused() or debugfs) dereference this freed memory and
cause a kernel panic?

> +	.use_rpm = true,

[Severity: High]
This is a pre-existing issue, but by setting use_rpm to true, does this
expose the driver to a power domain leak in the probe error path?

Looking at qcom_cc_really_probe() in drivers/clk/qcom/common.c, the error
path uses the asynchronous pm_runtime_put():

qcom_cc_really_probe() {
    ...
put_rpm:
	if (desc->use_rpm)
		pm_runtime_put(dev);

	return ret;
}

Since devm_pm_runtime_enable() is used, devres will automatically execute
pm_runtime_disable() on probe failure. 

Does this cancel the pending asynchronous idle/suspend work queued by
pm_runtime_put(), leaving the hardware permanently in an active state?

>  	.driver_data = &gcc_glymur_driver_data,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-glymur-fix-gcc-cx-scaling-v3-0-72eb5adad156@oss.qualcomm.com?part=2