Re: [PATCH AUTOSEL 6.18-6.6] drm/amd/pm: Check SMUv13.0.6/12 metrics integrity

[sashiko-bot@kernel.org]

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 <lijo.lazar@amd.com>

    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/20260831133314.4125787-482-sashal@kernel.org?part=1