Re: [PATCH] drm/amd/pm: bound SCLK FCW range entries to the table size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Unvalidated VBIOS array sizes and offsets cause out-of-bounds memory reads in atomctrl_get_leakage_vddc_base_on_leakage().
- [High] Missing VBIOS table size check in `atomctrl_get_avfs_information` allows out-of-bounds memory reads.
- [Medium] Fallback path uses uninitialized `smu_data->range_table` if the VBIOS table was successfully loaded.
- [Medium] The function `get_edc_leakage_table` correctly calculates the offset but incorrectly returns a pointer to the base table.
--

commit 79900a4165725430ba1e1ef41665d1982912cab7
Author: Hari Mishal <harimishal1@gmail.com>

drm/amd/pm: bound SCLK FCW range entries to the table size

This patch limits the number of entries read from the VBIOS SMU_Info table
into pp_atom_ctrl_sclk_range_table to MAX_SCLK_RANGE. It prevents a stack
buffer overflow caused by an unchecked ucSclkEntryNum count from the VBIOS.

> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c
> index 4b796d60b03d4..3217ad3c1d17e 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c
> @@ -1025,6 +1025,9 @@ int atomctrl_get_smc_sclk_range_table(struct pp_hwmgr *hwmgr, struct pp_atom_ctr
>  	if (!psmu_info)
>  		return -EINVAL;
>  
> +	if (psmu_info->ucSclkEntryNum > MAX_SCLK_RANGE)
> +		return -EINVAL;
> +
>  	for (i = 0; i < psmu_info->ucSclkEntryNum; i++) {
>  		table->entry[i].ucVco_setting = psmu_info->asSclkFcwRangeEntry[i].ucVco_setting;
>  		table->entry[i].ucPostdiv = psmu_info->asSclkFcwRangeEntry[i].ucPostdiv;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing VBIOS bounds
checking in ppatomctrl.c, I noticed another potential out-of-bounds read in
atomctrl_get_avfs_information():

drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c:atomctrl_get_avfs_information() {
	profile = (ATOM_ASIC_PROFILING_INFO_V3_6 *)
			smu_atom_get_data_table(hwmgr->adev,
					GetIndexIntoMasterTable(DATA, ASIC_ProfilingInfo),
					NULL, NULL, NULL);
	if (!profile)
		return -1;

	param->ulAVFS_meanNsigma_Acontant0 = le32_to_cpu(profile->ulAVFS_meanNsigma_Acontant0);
	...
}

Does this code fail to validate the size of the ASIC_ProfilingInfo table?

If a corrupted or older VBIOS provides a smaller table, could this lead to
reading past the end of the table mapping since the size parameter passed
to smu_atom_get_data_table() is NULL?

[Severity: High]
This is a pre-existing issue, but I found another unvalidated VBIOS array size
used in atomctrl_get_leakage_vddc_base_on_leakage():

drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c:atomctrl_get_leakage_vddc_base_on_leakage() {
	...
			for (i = 0; i < profile->ucElbVDDC_Num; i++) {
				if (vddc_id_buf[i] == virtual_voltage_id) {
	...
							*vddc = vddc_buf[j * profile->ucElbVDDC_Num + i];
	...
}

Can this loop iterate out-of-bounds if a malicious or corrupted VBIOS provides
an excessively large ucElbVDDC_Num?

The count and unvalidated offsets might lead to out-of-bounds memory reads.

[Severity: Medium]
This isn't a bug introduced by this patch, but get_edc_leakage_table() seems
to return the wrong pointer:

drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c:get_edc_leakage_table() {
	...
	temp = (char *)table_address;
	table_address += offset;

	return (AtomCtrl_EDCLeakgeTable *)temp;
}

Is this returning a pointer to the base table instead of the calculated offset?

Since table_address is incremented but temp is returned, might this cause
callers to read incorrect data from the beginning of the GFX_Info table rather
than the intended offset?

[Severity: Medium]
This is a pre-existing issue, but there seems to be an uninitialized memory
issue in the fallback path after loading the range table:

drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c:polaris10_get_sclk_range_table() {
	...
		}
		return;
	}
	...
}

drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c:polaris10_calculate_sclk_params() {
	...
		if (clock > smu_data->range_table[i].trans_lower_frequency
		&& clock <= smu_data->range_table[i].trans_upper_frequency) {
	...
}

If atomctrl_get_smc_sclk_range_table() succeeds,
polaris10_get_sclk_range_table() returns without initializing
smu_data->range_table.

If a subsequent divider calculation fails, could
polaris10_calculate_sclk_params() use this uninitialized range_table in its
fallback loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914132920.10127-1-harimishal1@gmail.com?part=1