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

[Feng, Kenneth]

[AMD Official Use Only - General]


[AMD Official Use Only - General]
Reviewed-by: Kenneth Feng <kenneth.feng at amd.com<mailto:kenneth.feng at amd.com>>


Best wishes
Kenneth Feng


发件人: Powell, Darren <Darren.Powell at amd.com>
日期: 星期四, 2022年6月16日 05:00
收件人: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
抄送: Lazar, Lijo <Lijo.Lazar at amd.com>, 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>
主题: Re: [PATCH v2 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2

[AMD Official Use Only - General]

Ping 🙂
________________________________
From: Powell, Darren <Darren.Powell at amd.com>
Sent: Wednesday, May 11, 2022 1:44 AM
To: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Lazar, Lijo <Lijo.Lazar at amd.com>; 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>; Powell, Darren <Darren.Powell at amd.com>
Subject: [PATCH v2 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2

[v2]
simplified fix after Lijo's feedback
 removed clocks.num_levels from calculation of loop count
   removed unsafe accesses to shim table freq_values
 retained corner case output only min,now if
   clocks.num_levels == 1 && now > min

 [v1]
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    | 34 +++++++------------
 1 file changed, 12 insertions(+), 22 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..3eb82bc88890 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -740,7 +740,7 @@ 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;
+       int display_levels;
         uint32_t freq_values[3] = {0};
         uint32_t min_clk, max_clk;

@@ -772,7 +772,7 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
                         return ret;
                 }

-               display_levels = clocks.num_levels;
+               display_levels = (clocks.num_levels == 1) ? 1 : 2;

                 min_clk = pstate_table->gfxclk_pstate.curr.min;
                 max_clk = pstate_table->gfxclk_pstate.curr.max;
@@ -782,30 +782,20 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,

                 /* fine-grained dpm has only 2 levels */
                 if (now > min_clk && now < max_clk) {
-                       display_levels = clocks.num_levels + 1;
+                       display_levels++;
                         freq_values[2] = max_clk;
                         freq_values[1] = now;
                 }

-               /*
-                * 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.num_levels == 1) ?
-                                               "*" :
-                                               (aldebaran_freqs_in_same_level(
-                                                        freq_values[i], now) ?
-                                                        "*" :
-                                                        ""));
-               } else {
-                       for (i = 0; i < display_levels; i++)
-                               size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
-                                               freq_values[i], i == 1 ? "*" : "");
-               }
+               for (i = 0; i < display_levels; i++)
+                       size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
+                               freq_values[i],
+                               (display_levels == 1) ?
+                                       "*" :
+                                       (aldebaran_freqs_in_same_level(
+                                                freq_values[i], now) ?
+                                                "*" :
+                                                ""));

                 break;

--
2.35.1
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220616/55a74eaa/attachment-0001.htm>