Messages in this thread | | | Date | Wed, 12 Apr 2023 05:39:04 -0700 | Subject | Re: [PATCH] radeon: avoid double free in ci_dpm_init() | From | Nikita Zhandarovich <> |
| |
On 4/11/23 14:11, Deucher, Alexander wrote: > [Public] > >> -----Original Message----- >> From: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >> Sent: Monday, April 3, 2023 2:28 PM >> To: Deucher, Alexander <Alexander.Deucher@amd.com> >> Cc: Nikita Zhandarovich <n.zhandarovich@fintech.ru>; Koenig, Christian >> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David >> Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd- >> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- >> kernel@vger.kernel.org; lvc-project@linuxtesting.org >> Subject: [PATCH] radeon: avoid double free in ci_dpm_init() >> >> There are several calls to ci_dpm_fini() in ci_dpm_init() when there occur >> errors in functions like r600_parse_extended_power_table(). >> This is harmful as it can lead to double free situations: for instance, >> r600_parse_extended_power_table() will call for >> r600_free_extended_power_table() as will ci_dpm_fini(), both of which will >> try to free resources. >> Other drivers do not call *_dpm_fini functions from their respective >> *_dpm_init calls - neither should cpm_dpm_init(). >> >> Fix this by removing extra calls to ci_dpm_fini(). > > You can't just drop the calls to fini(). You'll need to properly unwind to avoid leaking memory. > > Alex >>> >> Found by Linux Verification Center (linuxtesting.org) with static analysis tool >> SVACE. >> >> Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)") >> Cc: stable@vger.kernel.org >> Co-developed-by: Natalia Petrova <n.petrova@fintech.ru> >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >> >> --- >> drivers/gpu/drm/radeon/ci_dpm.c | 20 +++++--------------- >> 1 file changed, 5 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/ci_dpm.c >> b/drivers/gpu/drm/radeon/ci_dpm.c index 8ef25ab305ae..7b77d4c93f1d >> 100644 >> --- a/drivers/gpu/drm/radeon/ci_dpm.c >> +++ b/drivers/gpu/drm/radeon/ci_dpm.c >> @@ -5677,28 +5677,20 @@ int ci_dpm_init(struct radeon_device *rdev) >> pi->pcie_lane_powersaving.min = 16; >> >> ret = ci_get_vbios_boot_values(rdev, &pi->vbios_boot_state); >> - if (ret) { >> - ci_dpm_fini(rdev); >> + if (ret) >> return ret; >> - } >> >> ret = r600_get_platform_caps(rdev); >> - if (ret) { >> - ci_dpm_fini(rdev); >> + if (ret) >> return ret; >> - } >> >> ret = r600_parse_extended_power_table(rdev); >> - if (ret) { >> - ci_dpm_fini(rdev); >> + if (ret) >> return ret; >> - } >> >> ret = ci_parse_power_table(rdev); >> - if (ret) { >> - ci_dpm_fini(rdev); >> + if (ret) >> return ret; >> - } >> >> pi->dll_default_on = false; >> pi->sram_end = SMC_RAM_END; >> @@ -5749,10 +5741,8 @@ int ci_dpm_init(struct radeon_device *rdev) >> kcalloc(4, >> sizeof(struct >> radeon_clock_voltage_dependency_entry), >> GFP_KERNEL); >> - if (!rdev- >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) { >> - ci_dpm_fini(rdev); >> + if (!rdev- >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) >> return -ENOMEM; >> - } >> rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.count = 4; >> rdev- >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].clk = 0; >> rdev- >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].v = 0;
I think you are correct when it comes to ensuring we deal with memory issues in ci_dpm_init().
However, I could use some direction on how to deal with the problem of freeing only previously allocated resources. For instance, once ci_parse_power_table() fails, it is not clear what we should and should not free.
I wanna point out that in this case I would like to fix both double and uninitialized free issues as it can also lead to undefined behavior.
Thanks for your patience, Nikita
| |