2023-04-13 15:13:32

by Nikita Zhandarovich

[permalink] [raw]
Subject: [PATCH v2] radeon: avoid double free in ci_dpm_init()

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: [email protected]
Co-developed-by: Natalia Petrova <[email protected]>
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
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/[email protected]/

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;
}


2023-04-17 19:53:06

by Alex Deucher

[permalink] [raw]
Subject: 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
<[email protected]> 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: [email protected]
> Co-developed-by: Natalia Petrova <[email protected]>
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> 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/[email protected]/
>
> 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;
> }
>