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 <[email protected]> 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/[email protected]?part=1
