| Message ID | 20230403182808.8699-1-n.zhandarovich@fintech.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | radeon: avoid double free in ci_dpm_init() | expand |
[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;
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
On Wed, Apr 12, 2023 at 8:39 AM Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote: > > > > 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. 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
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;
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(). 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(-)