[PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2
Powell, Darren
Darren.Powell at amd.com
Tue May 10 22:07:16 UTC 2022
[AMD Official Use Only - General]
________________________________
From: Lazar, Lijo <Lijo.Lazar at amd.com>
Sent: Monday, May 9, 2022 12:50 AM
To: Powell, Darren <Darren.Powell at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Wang, Yang(Kevin) <KevinYang.Wang at amd.com>; Feng, Kenneth <Kenneth.Feng at amd.com>; Quan, Evan <Evan.Quan at amd.com>; Ma, Le <Le.Ma at amd.com>
Subject: Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2
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 at 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
[DP] I was concerned by the initialization of display_levels from clocks.num_levels and how it is used as the loop iterator
while accessing the freq_values array. My mistake was that I assumed that meant you intended to access clocks.data array.
If aldebaran only uses the curr.min and curr.max values that simplifies this greatly,
I'll respin this to initialize display_levels to 2 , and then output from freq_values array.
>
> - /*
> - * 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;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220510/3ae98349/attachment-0001.htm>
More information about the amd-gfx
mailing list