Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/amd/pm: NULL vdd_dep_on_sclk deref in get_pcie_table() and get_gpio_table() - [Medium] drm/amd/pm: 1-byte OOB read in get_tonga_sclk_dependency_table() and PCIE - [Medium] drm/amd/pm: missing PPTable and state array bounds in check_powerplay_tables() - [Medium] drm/amd/pm: unchecked ppt_get_vce_state_table_entry_v1_0() error in get_powerplay_table_entry_v1_0() - [Medium] drm/amd/pm: memory leak in pp_tables_v1_0_initialize() on subtable error --
commit f47b28bb12da9445f349f0b309395380a9db541f Author: Yang Wang <[email protected]> drm/amd/pm: Validate Tonga PPTable subtable bounds This commit adds bounds validation helpers for Tonga PowerPlay subtables to validate fixed subtable headers, dynamic entry arrays, and revision-specific layouts before consuming VBIOS or user-supplied PPTable data. > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c > b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c > index 6cfef1b295abb..a4cdae8ac73cf 100644 > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c > @@ -150,6 +150,368 @@ static const void *get_powerplay_table(struct pp_hwmgr > *hwmgr) > return table_address; > } > > +static bool tonga_pp_table_has_space(struct pp_hwmgr *hwmgr, size_t offset, > + size_t size) > +{ > + size_t table_size = hwmgr->soft_pp_table_size; > + > + return offset <= table_size && size <= table_size - offset; > +} [Severity: Medium] Can the new get_tonga_*_table() helpers and check_powerplay_tables() read out of bounds past hwmgr->soft_pp_table when hwmgr->soft_pp_table_size is smaller than sizeof(ATOM_Tonga_POWERPLAYTABLE), or when usStateArrayOffset and state_arrays->ucNumEntries exceed hwmgr->soft_pp_table_size? In drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c, get_powerplay_table() caches hwmgr->soft_pp_table_size from smu_atom_get_data_table() without checking that it is at least sizeof(ATOM_Tonga_POWERPLAYTABLE): if (!table_address) { table_address = (ATOM_Tonga_POWERPLAYTABLE *) smu_atom_get_data_table(hwmgr->adev, index, &size, &frev, &crev); hwmgr->soft_pp_table = table_address; /*Cache the result in RAM.*/ hwmgr->soft_pp_table_size = size; } All of the new get_tonga_*_table() subtable helpers dereference offset fields directly from powerplay_table (up to usGPIOTableOffset at the end of ATOM_Tonga_POWERPLAYTABLE) before calling tonga_pp_table_has_space(). In addition, because the prerequisite Tonga state-array bounds validation from the upstream series is not present on this branch, check_powerplay_tables(), get_number_of_powerplay_table_entries_v1_0(), and get_powerplay_table_entry_v1_0() still compute and dereference state_arrays without checking hwmgr->soft_pp_table_size: static int check_powerplay_tables( struct pp_hwmgr *hwmgr, const ATOM_Tonga_POWERPLAYTABLE *powerplay_table ) { const ATOM_Tonga_State_Array *state_arrays; state_arrays = (ATOM_Tonga_State_Array *)(((unsigned long)powerplay_table) + le16_to_cpu(powerplay_table->usStateArrayOffset)); ... PP_ASSERT_WITH_CODE((0 != powerplay_table->usStateArrayOffset), "State table is not set!", return -1); PP_ASSERT_WITH_CODE((0 < powerplay_table->sHeader.usStructureSize), "Invalid PowerPlay Table!", return -1); PP_ASSERT_WITH_CODE((0 < state_arrays->ucNumEntries), "Invalid PowerPlay Table!", return -1); Should the base ATOM_Tonga_POWERPLAYTABLE size and state_arrays bounds also be validated against hwmgr->soft_pp_table_size before dereferencing them? [ ... ] > +static int get_tonga_sclk_dependency_table(struct pp_hwmgr *hwmgr, > + const ATOM_Tonga_POWERPLAYTABLE *powerplay_table, > + const PPTable_Generic_SubTable_Header **sclk_dep_table) > +{ > + const PPTable_Generic_SubTable_Header *header; > + u16 table_offset; > + size_t entries_offset; > + size_t entry_size; > + u8 num_entries; > + int ret; > + > + table_offset = > le16_to_cpu(powerplay_table->usSclkDependencyTableOffset); > + ret = get_tonga_subtable(hwmgr, powerplay_table, table_offset, > + sizeof(*header), (const void **)&header); > + if (ret) > + return ret; > + > + if (header->ucRevId < 1) { > + const ATOM_Tonga_SCLK_Dependency_Table *table = > + (const ATOM_Tonga_SCLK_Dependency_Table *)header; > + > + entries_offset = offsetof(ATOM_Tonga_SCLK_Dependency_Table, > entries); > + entry_size = sizeof(ATOM_Tonga_SCLK_Dependency_Record); > + num_entries = table->ucNumEntries; > + } else { > + const ATOM_Polaris_SCLK_Dependency_Table *table = > + (const ATOM_Polaris_SCLK_Dependency_Table *)header; > + > + entries_offset = offsetof(ATOM_Polaris_SCLK_Dependency_Table, > entries); > + entry_size = sizeof(ATOM_Polaris_SCLK_Dependency_Record); > + num_entries = table->ucNumEntries; > + } > + > + ret = validate_tonga_table_entries(hwmgr, table_offset, entries_offset, > + num_entries, entry_size); > + if (ret) > + return ret; > + > + *sclk_dep_table = header; > + > + return 0; > +} > + > +static int get_tonga_pcie_table(struct pp_hwmgr *hwmgr, > + const ATOM_Tonga_POWERPLAYTABLE *powerplay_table, > + const PPTable_Generic_SubTable_Header **pcie_table) > +{ > + const PPTable_Generic_SubTable_Header *header; > + u16 table_offset; > + size_t entries_offset; > + size_t entry_size; > + u8 num_entries; > + int ret; > + > + table_offset = le16_to_cpu(powerplay_table->usPCIETableOffset); > + ret = get_tonga_subtable(hwmgr, powerplay_table, table_offset, > + sizeof(*header), (const void **)&header); > + if (ret) > + return ret; > + > + if (header->ucRevId < 1) { > + const ATOM_Tonga_PCIE_Table *table = > + (const ATOM_Tonga_PCIE_Table *)header; > + > + entries_offset = offsetof(ATOM_Tonga_PCIE_Table, entries); > + entry_size = sizeof(ATOM_Tonga_PCIE_Record); > + num_entries = table->ucNumEntries; > + } else { > + const ATOM_Polaris10_PCIE_Table *table = > + (const ATOM_Polaris10_PCIE_Table *)header; > + > + entries_offset = offsetof(ATOM_Polaris10_PCIE_Table, entries); > + entry_size = sizeof(ATOM_Polaris10_PCIE_Record); > + num_entries = table->ucNumEntries; > + } [Severity: Medium] Does reading table->ucNumEntries in get_tonga_sclk_dependency_table() and get_tonga_pcie_table() cause a 1-byte out-of-bounds read past hwmgr->soft_pp_table when table_offset equals hwmgr->soft_pp_table_size - 1? In drivers/gpu/drm/amd/pm/powerplay/hwmgr/pptable_v1_0.h, PPTable_Generic_SubTable_Header is only 1 byte: typedef struct _PPTable_Generic_SubTable_Header { UCHAR ucRevId; } PPTable_Generic_SubTable_Header; Both get_tonga_sclk_dependency_table() and get_tonga_pcie_table() pass sizeof(*header) (1 byte) to get_tonga_subtable(), and then immediately cast header to the concrete subtable type and read table->ucNumEntries at table_offset + 1 before calling validate_tonga_table_entries(). If usSclkDependencyTableOffset or usPCIETableOffset is hwmgr->soft_pp_table_size - 1, get_tonga_subtable() succeeds for the 1-byte header and table->ucNumEntries reads 1 byte past the end of hwmgr->soft_pp_table. [ ... ] > @@ -821,29 +1177,58 @@ static int init_clock_voltage_dependency( > pp_table_information->pcie_table = NULL; > pp_table_information->gpio_table = NULL; > > - if (powerplay_table->usMMDependencyTableOffset != 0) > - result = get_mm_clock_voltage_table(hwmgr, > - &pp_table_information->mm_dep_table, mm_dependency_table); > + if (powerplay_table->usMMDependencyTableOffset != 0) { > + result = get_tonga_mm_dependency_table(hwmgr, powerplay_table, > + &mm_dependency_table); > + if (!result) > + result = get_mm_clock_voltage_table(hwmgr, > + &pp_table_information->mm_dep_table, > + mm_dependency_table); > + } > > - if (result == 0 && powerplay_table->usPowerTuneTableOffset != 0) > - result = get_cac_tdp_table(hwmgr, > - &pp_table_information->cac_dtp_table, pPowerTuneTable); > + if (result == 0 && powerplay_table->usPowerTuneTableOffset != 0) { > + result = get_tonga_power_tune_table(hwmgr, powerplay_table, > + &pPowerTuneTable); > + if (!result) > + result = get_cac_tdp_table(hwmgr, > + &pp_table_information->cac_dtp_table, > + pPowerTuneTable); > + } > > - if (result == 0 && powerplay_table->usSclkDependencyTableOffset != 0) > - result = get_sclk_voltage_dependency_table(hwmgr, > - &pp_table_information->vdd_dep_on_sclk, sclk_dep_table); > + if (result == 0 && powerplay_table->usSclkDependencyTableOffset != 0) { > + result = get_tonga_sclk_dependency_table(hwmgr, powerplay_table, > + &sclk_dep_table); > + if (!result) > + result = get_sclk_voltage_dependency_table(hwmgr, > + &pp_table_information->vdd_dep_on_sclk, > + sclk_dep_table); > + } > > - if (result == 0 && powerplay_table->usMclkDependencyTableOffset != 0) > - result = get_mclk_voltage_dependency_table(hwmgr, > - &pp_table_information->vdd_dep_on_mclk, mclk_dep_table); > + if (result == 0 && powerplay_table->usMclkDependencyTableOffset != 0) { > + result = get_tonga_mclk_dependency_table(hwmgr, powerplay_table, > + &mclk_dep_table); > + if (!result) > + result = get_mclk_voltage_dependency_table(hwmgr, > + &pp_table_information->vdd_dep_on_mclk, > + mclk_dep_table); > + } > > - if (result == 0 && powerplay_table->usPCIETableOffset != 0) > - result = get_pcie_table(hwmgr, > - &pp_table_information->pcie_table, pcie_table); > + if (result == 0 && powerplay_table->usPCIETableOffset != 0) { > + result = get_tonga_pcie_table(hwmgr, powerplay_table, > + &pcie_table); > + if (!result) > + result = get_pcie_table(hwmgr, > + &pp_table_information->pcie_table, pcie_table); > + } > > - if (result == 0 && powerplay_table->usHardLimitTableOffset != 0) > - result = get_hard_limits(hwmgr, > - &pp_table_information->max_clock_voltage_on_dc, pHardLimits); > + if (result == 0 && powerplay_table->usHardLimitTableOffset != 0) { > + result = get_tonga_hard_limit_table(hwmgr, powerplay_table, > + &pHardLimits); > + if (!result) > + result = get_hard_limits(hwmgr, > + &pp_table_information->max_clock_voltage_on_dc, > + pHardLimits); > + } > > hwmgr->dyn_state.max_clock_voltage_on_dc.sclk = > pp_table_information->max_clock_voltage_on_dc.sclk; > @@ -864,9 +1249,13 @@ static int init_clock_voltage_dependency( > result = get_valid_clk(hwmgr, > &pp_table_information->valid_sclk_values, > pp_table_information->vdd_dep_on_sclk); > > - if (!result && gpio_table) > - result = get_gpio_table(hwmgr, > &pp_table_information->gpio_table, > - gpio_table); > + if (!result && powerplay_table->usGPIOTableOffset) { > + result = get_tonga_gpio_table(hwmgr, powerplay_table, > + &gpio_table); > + if (!result) > + result = get_gpio_table(hwmgr, > + &pp_table_information->gpio_table, gpio_table); > + } > > return result; > } [Severity: High] What happens in init_clock_voltage_dependency() if a malformed PPTable has usSclkDependencyTableOffset == 0 while usPCIETableOffset != 0 or usGPIOTableOffset != 0? In init_clock_voltage_dependency(), pp_table_information->vdd_dep_on_sclk is initialized to NULL and is only populated when powerplay_table->usSclkDependencyTableOffset != 0 (as anticipated before get_valid_clk(), which checks NULL != pp_table_information->vdd_dep_on_sclk). However, when usPCIETableOffset != 0 or usGPIOTableOffset != 0, get_pcie_table() and get_gpio_table() are called and unconditionally dereference pp_table_information->vdd_dep_on_sclk->count: get_pcie_table(): pcie_count = (pp_table_information->vdd_dep_on_sclk->count) + 1; get_gpio_table(): if (pp_table_information->vdd_dep_on_sclk->count < atom_gpio_table->ucVRHotTriggeredSclkDpmIndex) Can this trigger a NULL pointer dereference on a table with usSclkDependencyTableOffset == 0? [Severity: Medium] Also in init_clock_voltage_dependency(), init_dpm_2_parameters(), and init_thermal_controller(), if one of the new get_tonga_*() subtable bounds checks fails after earlier subtables have been allocated, does pp_tables_v1_0_initialize() leak hwmgr->pptable, hwmgr->dyn_state.cac_dtp_table, and the already-allocated subtable arrays in pp_table_information? When pp_tables_v1_0_initialize() returns a non-zero result: pp_tables_v1_0_initialize() { ... result = init_clock_voltage_dependency(hwmgr, powerplay_table); PP_ASSERT_WITH_CODE((result == 0), "init_clock_voltage_dependency failed", return result); result = init_dpm_2_parameters(hwmgr, powerplay_table); PP_ASSERT_WITH_CODE((result == 0), "init_dpm_2_parameters failed", return result); } its caller hwmgr_hw_init() in drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr.c jumps to err, bypassing err1 where pptable_fini() (pp_tables_v1_0_uninitialize()) would otherwise free those allocations: ret = hwmgr->pptable_func->pptable_init(hwmgr); if (ret) goto err; ... err1: if (hwmgr->pptable_func->pptable_fini) hwmgr->pptable_func->pptable_fini(hwmgr); err: return ret; [ ... ] > @@ -1290,13 +1683,15 @@ static int > ppt_get_num_of_vce_state_table_entries_v1_0(struct pp_hwmgr *hwmgr) > { > const ATOM_Tonga_POWERPLAYTABLE *pp_table = get_powerplay_table(hwmgr); > const ATOM_Tonga_VCE_State_Table *vce_state_table; > + int ret; > > > if (pp_table == NULL) > return 0; > > - vce_state_table = (void *)pp_table + > - le16_to_cpu(pp_table->usVCEStateTableOffset); > + ret = get_tonga_vce_state_table(hwmgr, pp_table, &vce_state_table); > + if (ret) > + return 0; > > return vce_state_table->ucNumEntries; > } > @@ -1305,18 +1700,39 @@ static int ppt_get_vce_state_table_entry_v1_0(struct > pp_hwmgr *hwmgr, uint32_t i > struct amd_vce_state *vce_state, void **clock_info, uint32_t > *flag) > { > const ATOM_Tonga_VCE_State_Record *vce_state_record; > - ATOM_Tonga_SCLK_Dependency_Record *sclk_dep_record; > + ATOM_Tonga_SCLK_Dependency_Record *sclk_dep_record = NULL; > + ATOM_Polaris_SCLK_Dependency_Record *polaris_sclk_dep_record = NULL; > ATOM_Tonga_MCLK_Dependency_Record *mclk_dep_record; > ATOM_Tonga_MM_Dependency_Record *mm_dep_record; > const ATOM_Tonga_POWERPLAYTABLE *pptable = get_powerplay_table(hwmgr); > - const ATOM_Tonga_VCE_State_Table *vce_state_table = > (ATOM_Tonga_VCE_State_Table *)(((unsigned long)pptable) > - + > le16_to_cpu(pptable->usVCEStateTableOffset)); > - const ATOM_Tonga_SCLK_Dependency_Table *sclk_dep_table = > (ATOM_Tonga_SCLK_Dependency_Table *)(((unsigned long)pptable) > - + > le16_to_cpu(pptable->usSclkDependencyTableOffset)); > - const ATOM_Tonga_MCLK_Dependency_Table *mclk_dep_table = > (ATOM_Tonga_MCLK_Dependency_Table *)(((unsigned long)pptable) > - + > le16_to_cpu(pptable->usMclkDependencyTableOffset)); > - const ATOM_Tonga_MM_Dependency_Table *mm_dep_table = > (ATOM_Tonga_MM_Dependency_Table *)(((unsigned long)pptable) > - + > le16_to_cpu(pptable->usMMDependencyTableOffset)); > + const ATOM_Tonga_VCE_State_Table *vce_state_table; > + const PPTable_Generic_SubTable_Header *sclk_dep_table_header; > + const ATOM_Tonga_SCLK_Dependency_Table *sclk_dep_table; > + const ATOM_Tonga_MCLK_Dependency_Table *mclk_dep_table; > + const ATOM_Tonga_MM_Dependency_Table *mm_dep_table; > + int ret; > + > + if (!pptable) > + return -EINVAL; > + > + ret = get_tonga_vce_state_table(hwmgr, pptable, &vce_state_table); > + if (ret) > + return ret; > + > + ret = get_tonga_sclk_dependency_table(hwmgr, pptable, > + &sclk_dep_table_header); > + if (ret) > + return ret; > + sclk_dep_table = (const ATOM_Tonga_SCLK_Dependency_Table *) > + sclk_dep_table_header; > + > + ret = get_tonga_mclk_dependency_table(hwmgr, pptable, &mclk_dep_table); > + if (ret) > + return ret; > + > + ret = get_tonga_mm_dependency_table(hwmgr, pptable, &mm_dep_table); > + if (ret) > + return ret; > > PP_ASSERT_WITH_CODE((i < vce_state_table->ucNumEntries), > "Requested state entry ID is out of range!", > @@ -1325,10 +1741,27 @@ static int ppt_get_vce_state_table_entry_v1_0(struct > pp_hwmgr *hwmgr, uint32_t i > vce_state_record = GET_FLEXIBLE_ARRAY_MEMBER_ADDR( > ATOM_Tonga_VCE_State_Record, > entries, vce_state_table, i); > - sclk_dep_record = GET_FLEXIBLE_ARRAY_MEMBER_ADDR( > - ATOM_Tonga_SCLK_Dependency_Record, > - entries, sclk_dep_table, > - vce_state_record->ucSCLKIndex); > + PP_ASSERT_WITH_CODE((vce_state_record->ucSCLKIndex < > + sclk_dep_table->ucNumEntries), > + "Invalid PowerPlay Table!", return -EINVAL); > + PP_ASSERT_WITH_CODE((vce_state_record->ucVCEClockIndex < > + mm_dep_table->ucNumEntries), > + "Invalid PowerPlay Table!", return -EINVAL); > + PP_ASSERT_WITH_CODE((mclk_dep_table->ucNumEntries != 0), > + "Invalid PowerPlay Table!", return -EINVAL); [Severity: Medium] When ppt_get_vce_state_table_entry_v1_0() returns early on one of these new validation checks, how does the caller get_powerplay_table_entry_v1_0() handle the failure? In get_powerplay_table_entry_v1_0(): hwmgr->num_vce_state_tables = i = ppt_get_num_of_vce_state_table_entries_v1_0(hwmgr); if ((i != 0) && (i <= AMD_MAX_VCE_LEVELS)) { for (j = 0; j < i; j++) ppt_get_vce_state_table_entry_v1_0(hwmgr, j, &(hwmgr->vce_states[j]), NULL, &flags); } ppt_get_num_of_vce_state_table_entries_v1_0() only validates get_tonga_vce_state_table() (and does not check ucNumEntries <= AMD_MAX_VCE_LEVELS). If ppt_get_vce_state_table_entry_v1_0() fails on get_tonga_sclk_dependency_table(), get_tonga_mclk_dependency_table(), get_tonga_mm_dependency_table(), or the ucSCLKIndex / ucVCEClockIndex range checks, its return value is ignored and hwmgr->num_vce_state_tables remains set to i while hwmgr->vce_states[j] is left unpopulated or retains stale data after a sysfs pp_table update. Similarly, if i > AMD_MAX_VCE_LEVELS, the loop is skipped while hwmgr->num_vce_state_tables remains set to i, which pp_dpm_get_vce_clock_state() in drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c then uses to bounds-check hwmgr->vce_states[idx]: if (idx < hwmgr->num_vce_state_tables) return &hwmgr->vce_states[idx]; Should get_powerplay_table_entry_v1_0() check the return value of ppt_get_vce_state_table_entry_v1_0() and reset hwmgr->num_vce_state_tables to 0 when i > AMD_MAX_VCE_LEVELS or when entry parsing fails? [ ... ] > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
