Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The patch corrupts the metrics cache on failure by overwriting it with poisoned data without invalidating the cache timestamp, allowing subsequent callers to read poisoned data as valid. Pre-existing issues: - [High] The return value of `smu_v13_0_6_setup_driver_pptable()` is ignored, which can lead to zero-initialized clock frequencies being used as valid DPM levels if the metrics table fetch fails. -- commit 6701a9c70e1205aa9e17d45906aef05c0bfc8e8f Author: Lijo Lazar <[email protected]> drm/amd/pm: Check SMUv13.0.6/12 metrics integrity Check if data fetch is proper by matching the first few bytes against 0xFFs. If 0xFFs, that means data couldn't be read properly. [ Upstream commit 6fc63d80c01a480e3d2c98620f07d46e5bcfc85a ] > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c > b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c > index 285cf79796939..c47beb75377ec 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c > @@ -768,6 +768,10 @@ int smu_v13_0_6_get_metrics_table(struct smu_context > *smu, void *metrics_table, > amdgpu_asic_invalidate_hdp(smu->adev, NULL); > memcpy(smu_table->metrics_table, table->cpu_addr, table_size); > > + if (!memchr_inv(smu_table->metrics_table, 0xff, > + min(16, table_size))) > + return -EHWPOISON; > + [Severity: High] Does this error path leave the metrics cache poisoned? If a caller (e.g., smu_v13_0_6_get_pm_metrics()) fetches the metrics table with bypass_cache = true, and the fetch fails this new integrity check, it will return -EHWPOISON. However, smu_table->metrics_table will already be overwritten with the poisoned (0xff) data, while smu_table->metrics_time remains unchanged. If a subsequent caller (e.g., smu_v13_0_6_get_gpu_metrics()) then requests metrics with bypass_cache = false within the 1ms window of the old timestamp, will it skip the fetch, copy the poisoned data, and return success? > smu_table->metrics_time = jiffies; > } > [Severity: High] This is a pre-existing issue, but can the ignored return value of smu_v13_0_6_setup_driver_pptable() cause zero-initialized clock frequencies to be used as valid DPM levels if the metrics table fetch fails? Looking at smu_v13_0_6_set_default_dpm_table(), the return value of smu_v13_0_6_setup_driver_pptable() is ignored: drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c:smu_v13_0_6_set_default_dpm_table() { ... smu_v13_0_6_setup_driver_pptable(smu); ... } If the metrics table fetch fails (e.g., due to the new integrity check returning -EHWPOISON), setup_driver_pptable() returns an error without populating the pptable structure. Because the return value is ignored, the caller iterates over the zero-initialized pptable and populates the DPM clock tables with 0 MHz: drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c:smu_v13_0_6_set_default_dpm_table() { ... for (j = 0; j < ARRAY_SIZE(dpm_map); j++) { ... dpm_table->count = levels; for (i = 0; i < dpm_table->count; ++i) { dpm_table->dpm_levels[i].value = dpm_map[j].freq_table[i]; dpm_table->dpm_levels[i].enabled = true; } ... } ... } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
