[Public]
Thanks for your suggestion, I will send the next version patch.
Regards,
Bob
-----Original Message-----
From: Huang, Tim <Tim.Huang@xxxxxxx>
Sent: 2024年5月31日 13:15
To: Zhou, Bob <Bob.Zhou@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Zhang, Jesse(Jie) <Jesse.Zhang@xxxxxxx>
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>; Zhou, Bob <Bob.Zhou@xxxxxxx>
Subject: RE: [PATCH] drm/amd/pm: Fix the null pointer dereference for vega10_hwmgr
[Public]
Hi Bob,
> -----Original Message-----
> From: Bob Zhou <bob.zhou@xxxxxxx>
> Sent: Wednesday, May 29, 2024 4:30 PM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Huang, Tim <Tim.Huang@xxxxxxx>;
> Zhang,
> Jesse(Jie) <Jesse.Zhang@xxxxxxx>
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Zhou, Bob <Bob.Zhou@xxxxxxx>
> Subject: [PATCH] drm/amd/pm: Fix the null pointer dereference for
> vega10_hwmgr
>
> Check return value and conduct null pointer handling to avoid null
> pointer dereference.
>
> Signed-off-by: Bob Zhou <bob.zhou@xxxxxxx>
> ---
> .../drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c | 38
> +++++++++++++++----
> 1 file changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> index 6524d99e5cab..0f94564b4adf 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> @@ -3436,16 +3436,20 @@ static int
> vega10_find_dpm_states_clocks_in_dpm_table(struct pp_hwmgr *hwmgr, co
> struct vega10_hwmgr *data = hwmgr->backend;
> const struct phm_set_power_state_input *states =
> (const struct phm_set_power_state_input *)input;
> - const struct vega10_power_state *vega10_ps =
> - cast_const_phw_vega10_power_state(states->pnew_state);
> + const struct vega10_power_state *vega10_ps;
Not revert xmas tree notation if change this.
> struct vega10_single_dpm_table *sclk_table =
> &(data->dpm_table.gfx_table);
> - uint32_t sclk = vega10_ps->performance_levels
> - [vega10_ps->performance_level_count - 1].gfx_clock;
> struct vega10_single_dpm_table *mclk_table =
> &(data->dpm_table.mem_table);
> - uint32_t mclk = vega10_ps->performance_levels
> - [vega10_ps->performance_level_count - 1].mem_clock;
> + uint32_t sclk, mclk;
> uint32_t i;
>
> + vega10_ps =
> + cast_const_phw_vega10_power_state(states->pnew_state);
Why need to change the definition of vega10_ps to two parts? It may be enough to only add below check and change the sclk and mclk.
> + if (vega10_ps == NULL)
> + return -EINVAL;
> + sclk = vega10_ps->performance_levels
> + [vega10_ps->performance_level_count - 1].gfx_clock;
> + mclk = vega10_ps->performance_levels
> + [vega10_ps->performance_level_count -
> + 1].mem_clock;
> +
> for (i = 0; i < sclk_table->count; i++) {
> if (sclk == sclk_table->dpm_levels[i].value)
> break;
> @@ -3748,10 +3752,13 @@ static int
> vega10_generate_dpm_level_enable_mask(
> struct vega10_hwmgr *data = hwmgr->backend;
> const struct phm_set_power_state_input *states =
> (const struct phm_set_power_state_input *)input;
> - const struct vega10_power_state *vega10_ps =
> - cast_const_phw_vega10_power_state(states->pnew_state);
> + const struct vega10_power_state *vega10_ps;
> int i;
>
> + vega10_ps =
> + cast_const_phw_vega10_power_state(states->pnew_state);
Same question as above, maybe it is enough to only add below check.
Tim Huang
> + if (vega10_ps == NULL)
> + return -EINVAL;
> +
> PP_ASSERT_WITH_CODE(!vega10_trim_dpm_states(hwmgr, vega10_ps),
> "Attempt to Trim DPM States Failed!",
> return -1);
> @@ -5036,6 +5043,9 @@ static int vega10_check_states_equal(struct
> pp_hwmgr *hwmgr,
> vega10_psa = cast_const_phw_vega10_power_state(pstate1);
> vega10_psb = cast_const_phw_vega10_power_state(pstate2);
>
> + if (vega10_psa == NULL || vega10_psb == NULL)
> + return -EINVAL;
> +
> /* If the two states don't even have the same number of performance levels
> * they cannot be the same state.
> */
> @@ -5168,6 +5178,8 @@ static int vega10_set_sclk_od(struct pp_hwmgr
> *hwmgr, uint32_t value)
> return -EINVAL;
>
> vega10_ps = cast_phw_vega10_power_state(&ps->hardware);
> + if (vega10_ps == NULL)
> + return -EINVAL;
>
> vega10_ps->performance_levels
> [vega10_ps->performance_level_count - 1].gfx_clock = @@ -5219,6
> +5231,8 @@ static int vega10_set_mclk_od(struct pp_hwmgr *hwmgr,
> +uint32_t
> value)
> return -EINVAL;
>
> vega10_ps = cast_phw_vega10_power_state(&ps->hardware);
> + if (vega10_ps == NULL)
> + return -EINVAL;
>
> vega10_ps->performance_levels
> [vega10_ps->performance_level_count - 1].mem_clock = @@ -5460,6
> +5474,9 @@ static void vega10_odn_update_power_state(struct pp_hwmgr
> *hwmgr)
> return;
>
> vega10_ps = cast_phw_vega10_power_state(&ps->hardware);
> + if (vega10_ps == NULL)
> + return;
> +
> max_level = vega10_ps->performance_level_count - 1;
>
> if (vega10_ps->performance_levels[max_level].gfx_clock != @@
> -5482,6
> +5499,9 @@ static void vega10_odn_update_power_state(struct pp_hwmgr
> *hwmgr)
>
> ps = (struct pp_power_state *)((unsigned long)(hwmgr->ps) +
> hwmgr->ps_size * (hwmgr->num_ps - 1));
> vega10_ps = cast_phw_vega10_power_state(&ps->hardware);
> + if (vega10_ps == NULL)
> + return;
> +
> max_level = vega10_ps->performance_level_count - 1;
>
> if (vega10_ps->performance_levels[max_level].gfx_clock != @@
> -5672,6
> +5692,8 @@ static int vega10_get_performance_level(struct pp_hwmgr
> *hwmgr, const struct pp_
> return -EINVAL;
>
> vega10_ps = cast_const_phw_vega10_power_state(state);
> + if (vega10_ps == NULL)
> + return -EINVAL;
>
> i = index > vega10_ps->performance_level_count - 1 ?
> vega10_ps->performance_level_count - 1 : index;
> --
> 2.34.1
