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 <[email protected]> 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/[email protected]?part=1
