Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 5/9/2022 9:28 AM, Darren Powell wrote:
>   added a check to populate and use SCLK shim table freq_values only
>     if using dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL or
>                           AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM
>   removed clocks.num_levels from calculation of shim table size
>   removed unsafe accesses to shim table freq_values
>     output gfx_table values if using other dpm levels
>   added check for freq_match when using freq_values for when now == min_clk
> 
> == Test ==
> LOGFILE=aldebaran-sclk.test.log
> AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1`
> AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | awk '{print $9}'`
> HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON}
> 
> lspci -nn | grep "VGA\|Display"  > $LOGFILE
> FILES="pp_od_clk_voltage
> pp_dpm_sclk"
> 
> for f in $FILES
> do
>    echo === $f === >> $LOGFILE
>    cat $HWMON_DIR/device/$f >> $LOGFILE
> done
> cat $LOGFILE
> 
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 60 +++++++++----------
>   1 file changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> index 6a4fca47ae53..a653668e8402 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -740,9 +740,8 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
>   	struct smu_13_0_dpm_table *single_dpm_table;
>   	struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
>   	struct smu_13_0_dpm_context *dpm_context = NULL;
> -	uint32_t display_levels;
>   	uint32_t freq_values[3] = {0};
> -	uint32_t min_clk, max_clk;
> +	uint32_t min_clk, max_clk, display_levels = 0;
>   
>   	smu_cmn_get_sysfs_buf(&buf, &size);
>   
> @@ -765,46 +764,45 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
>   			return ret;
>   		}
>   
> -		single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
> -		ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
> -		if (ret) {
> -			dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
> -			return ret;
> -		}
> -
> -		display_levels = clocks.num_levels;
> -
> -		min_clk = pstate_table->gfxclk_pstate.curr.min;
> -		max_clk = pstate_table->gfxclk_pstate.curr.max;
> -
> -		freq_values[0] = min_clk;
> -		freq_values[1] = max_clk;
> -
> -		/* fine-grained dpm has only 2 levels */
> -		if (now > min_clk && now < max_clk) {
> -			display_levels = clocks.num_levels + 1;
> -			freq_values[2] = max_clk;
> -			freq_values[1] = now;
> -		}
> +		if ((smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL &&
> +		     smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM)) {
> +			single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
> +			ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
> +			if (ret) {
> +				dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
> +				return ret;
> +			}

There are only two levels for GFX clock in aldebaran - min and max. 
Regardless of the mode, gfxclk_pstate.curr.min/max should reflect the 
current min/max level.

Could you explain the issue you are seeing? It's not so clear from the 
commit message.

Thanks,
Lijo

>   
> -		/*
> -		 * For DPM disabled case, there will be only one clock level.
> -		 * And it's safe to assume that is always the current clock.
> -		 */
> -		if (display_levels == clocks.num_levels) {
>   			for (i = 0; i < clocks.num_levels; i++)
>   				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
> -					freq_values[i],
> +					clocks.data[i].clocks_in_khz / 1000,
>   					(clocks.num_levels == 1) ?
>   						"*" :
>   						(aldebaran_freqs_in_same_level(
> -							 freq_values[i], now) ?
> +							 clocks.data[i].clocks_in_khz / 1000, now) ?
>   							 "*" :
>   							 ""));
>   		} else {
> +			/* fine-grained dpm has only 2 levels */
> +			display_levels = 2;
> +
> +			min_clk = pstate_table->gfxclk_pstate.curr.min;
> +			max_clk = pstate_table->gfxclk_pstate.curr.max;
> +
> +			freq_values[0] = min_clk;
> +			freq_values[1] = max_clk;
> +
> +			if (now > min_clk && now < max_clk) {
> +				display_levels++;
> +				freq_values[2] = max_clk;
> +				freq_values[1] = now;
> +			}
> +
>   			for (i = 0; i < display_levels; i++)
>   				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
> -						freq_values[i], i == 1 ? "*" : "");
> +						freq_values[i],
> +						aldebaran_freqs_in_same_level(freq_values[i], now) ?
> +							"*" : "");
>   		}
>   
>   		break;
>