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

[sashiko-bot@kernel.org]

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