[PATCH] drm/radeon: avoid double free in r600 DPM cleanup

[PATCH] drm/radeon: avoid double free in r600 DPM cleanup

[Ruoyu Wang]

r600_parse_extended_power_table() uses manual kfree() calls for some
early allocation failures, but the freed pointers are left in the
dynamic power-management state. If device teardown later calls
r600_free_extended_power_table(), those stale pointers can be freed
again.

Use the common extended power table cleanup helper for those early
failure paths as well, and clear each pointer after freeing it so
repeated cleanup stays safe.

Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/gpu/drm/radeon/r600_dpm.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r600_dpm.c b/drivers/gpu/drm/radeon/r600_dpm.c
index 83f1ae31cbdbc..9755e717ca8bb 100644
--- a/drivers/gpu/drm/radeon/r600_dpm.c
+++ b/drivers/gpu/drm/radeon/r600_dpm.c
@@ -932,7 +932,7 @@ int r600_parse_extended_power_table(struct radeon_device *rdev)
 			ret = r600_parse_clk_voltage_dep_table(&rdev->pm.dpm.dyn_state.vddci_dependency_on_mclk,
 							       dep_table);
 			if (ret) {
-				kfree(rdev->pm.dpm.dyn_state.vddc_dependency_on_sclk.entries);
+				r600_free_extended_power_table(rdev);
 				return ret;
 			}
 		}
@@ -943,8 +943,7 @@ int r600_parse_extended_power_table(struct radeon_device *rdev)
 			ret = r600_parse_clk_voltage_dep_table(&rdev->pm.dpm.dyn_state.vddc_dependency_on_mclk,
 							       dep_table);
 			if (ret) {
-				kfree(rdev->pm.dpm.dyn_state.vddc_dependency_on_sclk.entries);
-				kfree(rdev->pm.dpm.dyn_state.vddci_dependency_on_mclk.entries);
+				r600_free_extended_power_table(rdev);
 				return ret;
 			}
 		}
@@ -955,9 +954,7 @@ int r600_parse_extended_power_table(struct radeon_device *rdev)
 			ret = r600_parse_clk_voltage_dep_table(&rdev->pm.dpm.dyn_state.mvdd_dependency_on_mclk,
 							       dep_table);
 			if (ret) {
-				kfree(rdev->pm.dpm.dyn_state.vddc_dependency_on_sclk.entries);
-				kfree(rdev->pm.dpm.dyn_state.vddci_dependency_on_mclk.entries);
-				kfree(rdev->pm.dpm.dyn_state.vddc_dependency_on_mclk.entries);
+				r600_free_extended_power_table(rdev);
 				return ret;
 			}
 		}
@@ -1296,17 +1293,29 @@ void r600_free_extended_power_table(struct radeon_device *rdev)
 	struct radeon_dpm_dynamic_state *dyn_state = &rdev->pm.dpm.dyn_state;
 
 	kfree(dyn_state->vddc_dependency_on_sclk.entries);
+	dyn_state->vddc_dependency_on_sclk.entries = NULL;
 	kfree(dyn_state->vddci_dependency_on_mclk.entries);
+	dyn_state->vddci_dependency_on_mclk.entries = NULL;
 	kfree(dyn_state->vddc_dependency_on_mclk.entries);
+	dyn_state->vddc_dependency_on_mclk.entries = NULL;
 	kfree(dyn_state->mvdd_dependency_on_mclk.entries);
+	dyn_state->mvdd_dependency_on_mclk.entries = NULL;
 	kfree(dyn_state->cac_leakage_table.entries);
+	dyn_state->cac_leakage_table.entries = NULL;
 	kfree(dyn_state->phase_shedding_limits_table.entries);
+	dyn_state->phase_shedding_limits_table.entries = NULL;
 	kfree(dyn_state->ppm_table);
+	dyn_state->ppm_table = NULL;
 	kfree(dyn_state->cac_tdp_table);
+	dyn_state->cac_tdp_table = NULL;
 	kfree(dyn_state->vce_clock_voltage_dependency_table.entries);
+	dyn_state->vce_clock_voltage_dependency_table.entries = NULL;
 	kfree(dyn_state->uvd_clock_voltage_dependency_table.entries);
+	dyn_state->uvd_clock_voltage_dependency_table.entries = NULL;
 	kfree(dyn_state->samu_clock_voltage_dependency_table.entries);
+	dyn_state->samu_clock_voltage_dependency_table.entries = NULL;
 	kfree(dyn_state->acp_clock_voltage_dependency_table.entries);
+	dyn_state->acp_clock_voltage_dependency_table.entries = NULL;
 }
 
 enum radeon_pcie_gen r600_get_pcie_gen_support(struct radeon_device *rdev,
-- 
2.51.0

Re: [PATCH] drm/radeon: avoid double free in r600 DPM cleanup

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The return value of `radeon_dpm_init(rdev)` is ignored, leading to Use-After-Free and Double Free when initialization fails.
- [High] Stale array count fields lead to NULL pointer dereferences in downstream code.
--

commit ba5366568de66db75291f2bdbc278766341645c9
Author: Ruoyu Wang <ruoyuw560@gmail.com>

drm/radeon: avoid double free in r600 DPM cleanup
    
r600_parse_extended_power_table() uses manual kfree() calls for some
early allocation failures, but the freed pointers are left in the
dynamic power-management state. If device teardown later calls
r600_free_extended_power_table(), those stale pointers can be freed
again.

> diff --git a/drivers/gpu/drm/radeon/r600_dpm.c b/drivers/gpu/drm/radeon/r600_dpm.c
> index 81d58ef667dd4..2bc0bdfd11590 100644
> --- a/drivers/gpu/drm/radeon/r600_dpm.c
> +++ b/drivers/gpu/drm/radeon/r600_dpm.c
[ ... ]
> @@ -1298,17 +1295,29 @@ void r600_free_extended_power_table(struct radeon_device *rdev)
>  	struct radeon_dpm_dynamic_state *dyn_state = &rdev->pm.dpm.dyn_state;
>  
>  	kfree(dyn_state->vddc_dependency_on_sclk.entries);
> +	dyn_state->vddc_dependency_on_sclk.entries = NULL;

[Severity: Critical]
This is a pre-existing issue, but does the error handling in
radeon_pm_init_dpm() properly handle failures when these tables are freed?

It appears that the return value of radeon_dpm_init(rdev) is entirely ignored
in radeon_pm_init_dpm(). If initialization fails (e.g., in ci_dpm_init()
where it frees rdev->pm.dpm.priv via kfree()), the error is discarded and
the driver proceeds to execute radeon_dpm_setup_asic(rdev).

Downstream functions like ci_read_clock_registers() then retrieve the priv
pointer and write to it, resulting in a Use-After-Free:

drivers/gpu/drm/radeon/ci_dpm.c:ci_read_clock_registers() {
    struct ci_power_info *pi = ci_get_pi(rdev);
    ...
    pi->clock_registers.cg_spll_func_cntl = RREG32_SMC(CG_SPLL_FUNC_CNTL);
    ...
}

Additionally, upon driver removal, ci_dpm_fini() unconditionally calls
kfree(rdev->pm.dpm.priv), causing a double free.

[Severity: High]
This isn't a bug introduced by this patch, but does setting the entries
pointer to NULL without clearing the corresponding count field introduce a
NULL pointer dereference?

Because radeon_pm_init_dpm() ignores initialization failures, the driver
continues and calls functions like ci_populate_smc_initial_state() which
iterate based on the stale count field:

drivers/gpu/drm/radeon/ci_dpm.c:ci_populate_smc_initial_state() {
    ...
    for (level = 0;
         level < rdev->pm.dpm.dyn_state.vddc_dependency_on_sclk.count;
         level++) {
        if (rdev->pm.dpm.dyn_state.vddc_dependency_on_sclk.entries[level]...
    ...
}

Could the count fields also be set to 0 when freeing the entries? For example:
	dyn_state->vddc_dependency_on_sclk.count = 0;

>  	kfree(dyn_state->vddci_dependency_on_mclk.entries);
> +	dyn_state->vddci_dependency_on_mclk.entries = NULL;
>  	kfree(dyn_state->vddc_dependency_on_mclk.entries);
> +	dyn_state->vddc_dependency_on_mclk.entries = NULL;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608063638.24-1-ruoyuw560@gmail.com?part=1