Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230413151228.19714-1-n.zhandarovich@fintech.ru>
In-Reply-To: <20230413151228.19714-1-n.zhandarovich@fintech.ru>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Mon, 17 Apr 2023 15:34:18 -0400
Message-ID: <CADnq5_MqJ7jnYrRus+HFy+Qk5F6b3627TN-gpxSbGgPdgfLkfw@mail.gmail.com>
Subject: Re: [PATCH v2] radeon: avoid double free in ci_dpm_init()
To:     Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Cc:     Alex Deucher <alexander.deucher@amd.com>,
        Natalia Petrova <n.petrova@fintech.ru>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>, linux-kernel@vger.kernel.org,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        stable@vger.kernel.org,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        lvc-project@linuxtesting.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Thanks.  Applied!

Alex

On Thu, Apr 13, 2023 at 11:12=E2=80=AFAM 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@fint=
ech.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_devic=
e *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_dev=
ice *rdev)
>                 non_clock_array_index =3D power_state->v2.nonClockInfoInd=
ex;
>                 non_clock_info =3D (struct _ATOM_PPLIB_NONCLOCK_INFO *)
>                         &non_clock_info_array->nonClockInfo[non_clock_arr=
ay_index];
> -               if (!rdev->pm.power_state[i].clock_info)
> -                       return -EINVAL;
> +               if (!rdev->pm.power_state[i].clock_info) {
> +                       ret =3D -EINVAL;
> +                       goto err_free_ps;
> +               }
>                 ps =3D kzalloc(sizeof(struct ci_ps), GFP_KERNEL);
> -               if (ps =3D=3D NULL)
> -                       return -ENOMEM;
> +               if (ps =3D=3D NULL) {
> +                       ret =3D -ENOMEM;
> +                       goto err_free_ps;
> +               }
>                 rdev->pm.dpm.ps[i].ps_priv =3D 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_devi=
ce *rdev)
>         }
>
>         return 0;
> +
> +err_free_ps:
> +       for (i =3D 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 =3D ci_get_vbios_boot_values(rdev, &pi->vbios_boot_state);
>         if (ret) {
> -               ci_dpm_fini(rdev);
> +               kfree(rdev->pm.dpm.priv);
>                 return ret;
>         }
>
>         ret =3D r600_get_platform_caps(rdev);
>         if (ret) {
> -               ci_dpm_fini(rdev);
> +               kfree(rdev->pm.dpm.priv);
>                 return ret;
>         }
>
>         ret =3D r600_parse_extended_power_table(rdev);
>         if (ret) {
> -               ci_dpm_fini(rdev);
> +               kfree(rdev->pm.dpm.priv);
>                 return ret;
>         }
>
>         ret =3D ci_parse_power_table(rdev);
>         if (ret) {
> -               ci_dpm_fini(rdev);
> +               kfree(rdev->pm.dpm.priv);
> +               r600_free_extended_power_table(rdev);
>                 return ret;
>         }
>