Re: [PATCH] radeon: avoid double free in ci_dpm_init()
On Wed, Apr 12, 2023 at 8:39 AM Nikita Zhandarovich wrote: > > > > On 4/11/23 14:11, Deucher, Alexander wrote: > > [Public] > > > >> -Original Message- > >> From: Nikita Zhandarovich > >> Sent: Monday, April 3, 2023 2:28 PM > >> To: Deucher, Alexander > >> Cc: Nikita Zhandarovich ; Koenig, Christian > >> ; Pan, Xinhui ; David > >> Airlie ; Daniel Vetter ; amd- > >> g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- > >> ker...@vger.kernel.org; lvc-proj...@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: sta...@vger.kernel.org > >> Co-developed-by: Natalia Petrova > >> Signed-off-by: Nikita Zhandarovich > >> > >> --- > >> 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, >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. You'll want to free any memory allocated in ci_dpm_init(). Any of the functions called from that function should clean themselves up if they allocate any memory, but if not, they should be fixed. Alex > > 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
Re: [PATCH] radeon: avoid double free in ci_dpm_init()
On 4/11/23 14:11, Deucher, Alexander wrote: > [Public] > >> -Original Message- >> From: Nikita Zhandarovich >> Sent: Monday, April 3, 2023 2:28 PM >> To: Deucher, Alexander >> Cc: Nikita Zhandarovich ; Koenig, Christian >> ; Pan, Xinhui ; David >> Airlie ; Daniel Vetter ; amd- >> g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- >> ker...@vger.kernel.org; lvc-proj...@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: sta...@vger.kernel.org >> Co-developed-by: Natalia Petrova >> Signed-off-by: Nikita Zhandarovich >> >> --- >> 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, >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
RE: [PATCH] radeon: avoid double free in ci_dpm_init()
[Public] > -Original Message- > From: Nikita Zhandarovich > Sent: Monday, April 3, 2023 2:28 PM > To: Deucher, Alexander > Cc: Nikita Zhandarovich ; Koenig, Christian > ; Pan, Xinhui ; David > Airlie ; Daniel Vetter ; amd- > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- > ker...@vger.kernel.org; lvc-proj...@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: sta...@vger.kernel.org > Co-developed-by: Natalia Petrova > Signed-off-by: Nikita Zhandarovich > > --- > 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, >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;