Re: [PATCH v2] radeon: avoid double free in ci_dpm_init()

Thanks. Applied!

Alex

On Thu, Apr 13, 2023 at 11:12 AM Nikita Zhandarovich n.zhandarovich@fintech.ru wrote:

Several calls to ci_dpm_fini() will attempt to free resources that either have been freed before or haven't been allocated yet. This may lead to undefined or dangerous behaviour.

For instance, if r600_parse_extended_power_table() fails, it might call r600_free_extended_power_table() as will ci_dpm_fini() later during error handling.

Fix this by only freeing pointers to objects previously allocated.

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

---

v2: free only resouces allocated prior, do not remove ci_dpm_fini() or other deallocating calls altogether; fix commit message. v1: https://lore.kernel.org/all/20230403182808.8699-1-n.zhandarovich@fintech.ru/

drivers/gpu/drm/radeon/ci_dpm.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c index 8ef25ab305ae..b8f4dac68d85 100644 --- a/drivers/gpu/drm/radeon/ci_dpm.c +++ b/drivers/gpu/drm/radeon/ci_dpm.c @@ -5517,6 +5517,7 @@ static int ci_parse_power_table(struct radeon_device *rdev) u8 frev, crev; u8 *power_state_offset; struct ci_ps *ps;

•   int ret;
  
    if (!atom_parse_data_header(mode_info->atom_context, index, NULL,
                               &frev, &crev, &data_offset))
  

@@ -5546,11 +5547,15 @@ static int ci_parse_power_table(struct radeon_device *rdev) non_clock_array_index = power_state->v2.nonClockInfoIndex; non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *) &non_clock_info_array->nonClockInfo[non_clock_array_index];

•           if (!rdev->pm.power_state[i].clock_info)
  

•                   return -EINVAL;
  

•           if (!rdev->pm.power_state[i].clock_info) {
  

•                   ret = -EINVAL;
  

•                   goto err_free_ps;
  

•           }
            ps = kzalloc(sizeof(struct ci_ps), GFP_KERNEL);
  

•           if (ps == NULL)
  

•                   return -ENOMEM;
  

•           if (ps == NULL) {
  

•                   ret = -ENOMEM;
  

•                   goto err_free_ps;
  

•           }
            rdev->pm.dpm.ps[i].ps_priv = ps;
            ci_parse_pplib_non_clock_info(rdev, &rdev->pm.dpm.ps[i],
                                          non_clock_info,
  

@@ -5590,6 +5595,12 @@ static int ci_parse_power_table(struct radeon_device *rdev) }

    return 0;

+err_free_ps:

•   for (i = 0; i < rdev->pm.dpm.num_ps; i++)
  

•           kfree(rdev->pm.dpm.ps[i].ps_priv);
  

•   kfree(rdev->pm.dpm.ps);
  

•   return ret;
  

}

static int ci_get_vbios_boot_values(struct radeon_device *rdev, @@ -5678,25 +5689,26 @@ int ci_dpm_init(struct radeon_device *rdev)

    ret = ci_get_vbios_boot_values(rdev, &pi->vbios_boot_state);
    if (ret) {

•           ci_dpm_fini(rdev);
  

•           kfree(rdev->pm.dpm.priv);
            return ret;
    }
  
    ret = r600_get_platform_caps(rdev);
    if (ret) {
  

•           ci_dpm_fini(rdev);
  

•           kfree(rdev->pm.dpm.priv);
            return ret;
    }
  
    ret = r600_parse_extended_power_table(rdev);
    if (ret) {
  

•           ci_dpm_fini(rdev);
  

•           kfree(rdev->pm.dpm.priv);
            return ret;
    }
  
    ret = ci_parse_power_table(rdev);
    if (ret) {
  

•           ci_dpm_fini(rdev);
  

•           kfree(rdev->pm.dpm.priv);
  

•           r600_free_extended_power_table(rdev);
            return ret;
    }