Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/amd/pm: unvalidated subtable offsets in init_clock_voltage_dependency() - [High] drm/amd/pm: missing subtable bounds checks in pp_tables_initialize() - [High] drm/amd/pm: weak clock info size and level checks in pp_tables_get_entry() --
commit c5c1e3659fd69a3025883887b44b7fe3a2082d39 Author: Yang Wang <[email protected]> drm/amd/pm: Validate legacy hwmgr PP table offsets This commit adds bounds helpers to drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c to validate offsets and entry sizes against hwmgr->soft_pp_table_size before dereferencing variable-length PowerPlay subtables during hwmgr initialization and table entry lookup. > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c > b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c > index f06b29e33ba45..f1009b2973574 100644 > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c [ ... ] > @@ -104,10 +138,13 @@ static uint16_t > get_vce_clock_voltage_limit_table_offset(struct pp_hwmgr *hwmgr, > { > uint16_t table_offset = get_vce_clock_info_array_offset(hwmgr, > powerplay_table); > + u16 table_size; > > - if (table_offset > 0) > - return table_offset + get_vce_clock_info_array_size(hwmgr, > - powerplay_table); > + if (table_offset > 0) { > + table_size = get_vce_clock_info_array_size(hwmgr, > powerplay_table); > + if (table_size) > + return table_offset + table_size; > + } > > return 0; > } [ ... ] > @@ -206,10 +256,13 @@ static uint16_t > get_uvd_clock_voltage_limit_table_offset( > { > uint16_t table_offset = get_uvd_clock_info_array_offset(hwmgr, > powerplay_table); > + u16 table_size; > > - if (table_offset > 0) > - return table_offset + > - get_uvd_clock_info_array_size(hwmgr, powerplay_table); > + if (table_offset > 0) { > + table_size = get_uvd_clock_info_array_size(hwmgr, > powerplay_table); > + if (table_size) > + return table_offset + table_size; > + } > > return 0; > } > @@ -218,21 +271,12 @@ static uint16_t get_samu_table_offset(struct pp_hwmgr > *hwmgr, > const ATOM_PPLIB_POWERPLAYTABLE *powerplay_table) > { > uint16_t samu_table_offset = 0; > + const ATOM_PPLIB_EXTENDEDHEADER *extended_header; > > - if (le16_to_cpu(powerplay_table->usTableSize) >= > - sizeof(ATOM_PPLIB_POWERPLAYTABLE3)) { > - const ATOM_PPLIB_POWERPLAYTABLE3 *powerplay_table3 = > - (const ATOM_PPLIB_POWERPLAYTABLE3 *)powerplay_table; > - if (powerplay_table3->usExtendendedHeaderOffset > 0) { > - const ATOM_PPLIB_EXTENDEDHEADER *extended_header = > - (const ATOM_PPLIB_EXTENDEDHEADER *) > - (((unsigned long)powerplay_table3) + > - > le16_to_cpu(powerplay_table3->usExtendendedHeaderOffset)); > - if (le16_to_cpu(extended_header->usSize) >= > - SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V4) > - samu_table_offset = > le16_to_cpu(extended_header->usSAMUTableOffset); > - } > - } > + extended_header = get_extended_header(hwmgr, powerplay_table, > + > SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V4); > + if (extended_header) > + samu_table_offset = > le16_to_cpu(extended_header->usSAMUTableOffset); > > return samu_table_offset; > } > @@ -254,21 +298,12 @@ static uint16_t get_acp_table_offset(struct pp_hwmgr > *hwmgr, > const ATOM_PPLIB_POWERPLAYTABLE > *powerplay_table) > { > uint16_t acp_table_offset = 0; > + const ATOM_PPLIB_EXTENDEDHEADER *extended_header; > > - if (le16_to_cpu(powerplay_table->usTableSize) >= > - sizeof(ATOM_PPLIB_POWERPLAYTABLE3)) { > - const ATOM_PPLIB_POWERPLAYTABLE3 *powerplay_table3 = > - (const ATOM_PPLIB_POWERPLAYTABLE3 *)powerplay_table; > - if (powerplay_table3->usExtendendedHeaderOffset > 0) { > - const ATOM_PPLIB_EXTENDEDHEADER *pExtendedHeader = > - (const ATOM_PPLIB_EXTENDEDHEADER *) > - (((unsigned long)powerplay_table3) + > - > le16_to_cpu(powerplay_table3->usExtendendedHeaderOffset)); > - if (le16_to_cpu(pExtendedHeader->usSize) >= > - SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V6) > - acp_table_offset = > le16_to_cpu(pExtendedHeader->usACPTableOffset); > - } > - } > + extended_header = get_extended_header(hwmgr, powerplay_table, > + > SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V6); > + if (extended_header) > + acp_table_offset = > le16_to_cpu(extended_header->usACPTableOffset); > > return acp_table_offset; > } > @@ -290,21 +325,12 @@ static uint16_t get_cacp_tdp_table_offset( > const ATOM_PPLIB_POWERPLAYTABLE > *powerplay_table) > { > uint16_t cacTdpTableOffset = 0; > + const ATOM_PPLIB_EXTENDEDHEADER *extended_header; > > - if (le16_to_cpu(powerplay_table->usTableSize) >= > - sizeof(ATOM_PPLIB_POWERPLAYTABLE3)) { > - const ATOM_PPLIB_POWERPLAYTABLE3 *powerplay_table3 = > - (const ATOM_PPLIB_POWERPLAYTABLE3 > *)powerplay_table; > - if (powerplay_table3->usExtendendedHeaderOffset > 0) { > - const ATOM_PPLIB_EXTENDEDHEADER *pExtendedHeader = > - (const ATOM_PPLIB_EXTENDEDHEADER *) > - (((unsigned long)powerplay_table3) + > - > le16_to_cpu(powerplay_table3->usExtendendedHeaderOffset)); > - if (le16_to_cpu(pExtendedHeader->usSize) >= > - SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V7) > - cacTdpTableOffset = > le16_to_cpu(pExtendedHeader->usPowerTuneTableOffset); > - } > - } > + extended_header = get_extended_header(hwmgr, powerplay_table, > + > SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V7); > + if (extended_header) > + cacTdpTableOffset = > le16_to_cpu(extended_header->usPowerTuneTableOffset); > > return cacTdpTableOffset; > } > @@ -341,22 +367,13 @@ static uint16_t get_sclk_vdd_gfx_table_offset(struct > pp_hwmgr *hwmgr, > const ATOM_PPLIB_POWERPLAYTABLE *powerplay_table) > { > uint16_t sclk_vdd_gfx_table_offset = 0; > + const ATOM_PPLIB_EXTENDEDHEADER *extended_header; > > - if (le16_to_cpu(powerplay_table->usTableSize) >= > - sizeof(ATOM_PPLIB_POWERPLAYTABLE3)) { > - const ATOM_PPLIB_POWERPLAYTABLE3 *powerplay_table3 = > - (const ATOM_PPLIB_POWERPLAYTABLE3 > *)powerplay_table; > - if (powerplay_table3->usExtendendedHeaderOffset > 0) { > - const ATOM_PPLIB_EXTENDEDHEADER *pExtendedHeader = > - (const ATOM_PPLIB_EXTENDEDHEADER *) > - (((unsigned long)powerplay_table3) + > - > le16_to_cpu(powerplay_table3->usExtendendedHeaderOffset)); > - if (le16_to_cpu(pExtendedHeader->usSize) >= > - SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V8) > - sclk_vdd_gfx_table_offset = > - > le16_to_cpu(pExtendedHeader->usSclkVddgfxTableOffset); > - } > - } > + extended_header = get_extended_header(hwmgr, powerplay_table, > + > SIZE_OF_ATOM_PPLIB_EXTENDEDHEADER_V8); > + if (extended_header) > + sclk_vdd_gfx_table_offset = > + le16_to_cpu(extended_header->usSclkVddgfxTableOffset); > > return sclk_vdd_gfx_table_offset; > } [Severity: High] Can init_clock_voltage_dependency() still read past hwmgr->soft_pp_table when using the offsets returned by these helpers? In get_vce_clock_voltage_limit_table_offset() and get_uvd_clock_voltage_limit_table_offset(), the new check verifies that the preceding VCEClockInfoArray or UVDClockInfoArray fits within hwmgr->soft_pp_table_size, and then returns table_offset + table_size. If that clock info array ends at hwmgr->soft_pp_table_size (or leaves fewer bytes than the voltage limit table requires), the returned offset is non-zero and points at or near the end of soft_pp_table. When pp_tables_initialize() -> init_clock_voltage_dependency() uses those offsets, it does not call get_vce_clock_voltage_limit_table_size() or pp_table_has_space(): drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c:init_clock_voltage_dependency() { vce_clock_info_array_offset = get_vce_clock_info_array_offset( hwmgr, powerplay_table); table_offset = get_vce_clock_voltage_limit_table_offset(hwmgr, powerplay_table); if (vce_clock_info_array_offset > 0 && table_offset > 0) { const VCEClockInfoArray *array = (const VCEClockInfoArray *) (((unsigned long) powerplay_table) + vce_clock_info_array_offset); const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *table = (const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *) (((unsigned long) powerplay_table) + table_offset); result = get_vce_clock_voltage_limit_table(hwmgr, &hwmgr->dyn_state.vce_clock_voltage_dependency_table, table, array); } } Then get_vce_clock_voltage_limit_table() and get_uvd_clock_voltage_limit_table() dereference table->numEntries and table->entries[i] without bounds checks, and also index array->entries[table->entries[i].ucVCEClockInfoIndex] and array->entries[table->entries[i].ucUVDClockInfoIndex] without verifying that the index is less than array->ucNumEntries. Similarly, get_samu_table_offset(), get_acp_table_offset(), get_cacp_tdp_table_offset(), and get_sclk_vdd_gfx_table_offset() now validate the extended header via get_extended_header(), but neither those helpers nor their sole caller init_clock_voltage_dependency() verify that the returned subtable offsets and their entry arrays fit within hwmgr->soft_pp_table_size before dereferencing them. [ ... ] > @@ -896,49 +940,128 @@ int pp_tables_get_entry(struct pp_hwmgr *hwmgr, > const NonClockInfoArray *pnon_clock_arrays; > > const ATOM_PPLIB_STATE *pstate_entry; > + u16 state_array_offset; > + u16 clock_info_array_offset; > + u16 non_clock_info_array_offset; > + size_t clock_info_offset; > + size_t non_clock_info_offset; > + size_t state_entry_offset; > > if (powerplay_table == NULL) > return -1; > + if (!pp_table_has_space(hwmgr, 0, sizeof(*powerplay_table))) > + return -1; > > ps->classification.bios_index = entry_index; > > if (powerplay_table->sHeader.ucTableFormatRevision >= 6) { > + state_array_offset = > le16_to_cpu(powerplay_table->usStateArrayOffset); > + if (!pp_table_has_space(hwmgr, state_array_offset, > + sizeof(pstate_arrays->ucNumEntries))) > + return -1; > + > pstate_arrays = (StateArray *)(((unsigned long)powerplay_table) > + > - > le16_to_cpu(powerplay_table->usStateArrayOffset)); > + state_array_offset); > + > + if (entry_index >= pstate_arrays->ucNumEntries) > + return -1; > > - if (entry_index > pstate_arrays->ucNumEntries) > + pstate_entry_v2 = get_state_entry_v2(hwmgr, pstate_arrays, > + state_array_offset, > + entry_index); > + if (!pstate_entry_v2) > + return -1; > + > + clock_info_array_offset = > + le16_to_cpu(powerplay_table->usClockInfoArrayOffset); > + if (!pp_table_has_space(hwmgr, clock_info_array_offset, > + sizeof(*pclock_arrays))) > return -1; > > - pstate_entry_v2 = get_state_entry_v2(pstate_arrays, > entry_index); > pclock_arrays = (ClockInfoArray *)(((unsigned > long)powerplay_table) + > - > le16_to_cpu(powerplay_table->usClockInfoArrayOffset)); > + clock_info_array_offset); > + if (!pclock_arrays->ucEntrySize) > + return -1; > + > + non_clock_info_array_offset = > + le16_to_cpu(powerplay_table->usNonClockInfoArrayOffset); > + if (!pp_table_has_space(hwmgr, non_clock_info_array_offset, > + sizeof(*pnon_clock_arrays))) > + return -1; > > pnon_clock_arrays = (NonClockInfoArray *)(((unsigned > long)powerplay_table) + > - > le16_to_cpu(powerplay_table->usNonClockInfoArrayOffset)); > + non_clock_info_array_offset); > + if (!pnon_clock_arrays->ucEntrySize || > + pnon_clock_arrays->ucEntrySize < > ATOM_PPLIB_NONCLOCKINFO_VER1 || > + (pnon_clock_arrays->ucEntrySize > > ATOM_PPLIB_NONCLOCKINFO_VER1 && > + pnon_clock_arrays->ucEntrySize < > ATOM_PPLIB_NONCLOCKINFO_VER2) || > + pstate_entry_v2->nonClockInfoIndex >= > pnon_clock_arrays->ucNumEntries) > + return -1; > > + non_clock_info_offset = non_clock_info_array_offset + > + offsetof(NonClockInfoArray, nonClockInfo) + > + pstate_entry_v2->nonClockInfoIndex * > pnon_clock_arrays->ucEntrySize; > + if (!pp_table_has_space(hwmgr, non_clock_info_offset, > + pnon_clock_arrays->ucEntrySize)) > + return -1; > pnon_clock_info = (ATOM_PPLIB_NONCLOCK_INFO *)((unsigned > long)(pnon_clock_arrays->nonClockInfo) + > (pstate_entry_v2->nonClockInfoIndex * > pnon_clock_arrays->ucEntrySize)); > > result = init_non_clock_fields(hwmgr, ps, > pnon_clock_arrays->ucEntrySize, pnon_clock_info); > > for (i = 0; i < pstate_entry_v2->ucNumDPMLevels; i++) { > - const void *pclock_info = (const void *)( > - (unsigned > long)(pclock_arrays->clockInfo) + > - > (pstate_entry_v2->clockInfoIndex[i] * pclock_arrays->ucEntrySize)); > + const void *pclock_info; > + > + if (pstate_entry_v2->clockInfoIndex[i] >= > + pclock_arrays->ucNumEntries) > + return -1; > + > + clock_info_offset = clock_info_array_offset + > + offsetof(ClockInfoArray, clockInfo) + > + pstate_entry_v2->clockInfoIndex[i] * > pclock_arrays->ucEntrySize; > + if (!pp_table_has_space(hwmgr, clock_info_offset, > + pclock_arrays->ucEntrySize)) > + return -1; > + > + pclock_info = (const void *) > + ((unsigned > long)(pclock_arrays->clockInfo) + > + (pstate_entry_v2->clockInfoIndex[i] * > + pclock_arrays->ucEntrySize)); > res = func(hwmgr, &ps->hardware, i, pclock_info); > if ((0 == result) && (0 != res)) > result = res; > } > } else { > - if (entry_index > powerplay_table->ucNumStates) > + if (entry_index >= powerplay_table->ucNumStates || > + !powerplay_table->ucStateEntrySize || > + !powerplay_table->ucNonClockSize || > + powerplay_table->ucNonClockSize < > ATOM_PPLIB_NONCLOCKINFO_VER1 || > + (powerplay_table->ucNonClockSize > > ATOM_PPLIB_NONCLOCKINFO_VER1 && > + powerplay_table->ucNonClockSize < > ATOM_PPLIB_NONCLOCKINFO_VER2) || > + !powerplay_table->ucClockInfoSize) > + return -1; > + > + state_array_offset = > le16_to_cpu(powerplay_table->usStateArrayOffset); > + state_entry_offset = state_array_offset + > + entry_index * powerplay_table->ucStateEntrySize; > + if (!pp_table_has_space(hwmgr, state_entry_offset, > + powerplay_table->ucStateEntrySize)) > return -1; > > pstate_entry = (ATOM_PPLIB_STATE *)((unsigned > long)powerplay_table + > - > le16_to_cpu(powerplay_table->usStateArrayOffset) + > + state_array_offset + > entry_index * > powerplay_table->ucStateEntrySize); > > + non_clock_info_array_offset = > + le16_to_cpu(powerplay_table->usNonClockInfoArrayOffset); > + non_clock_info_offset = non_clock_info_array_offset + > + pstate_entry->ucNonClockStateIndex * > powerplay_table->ucNonClockSize; > + if (!pp_table_has_space(hwmgr, non_clock_info_offset, > + powerplay_table->ucNonClockSize)) > + return -1; > + > pnon_clock_info = (ATOM_PPLIB_NONCLOCK_INFO *)((unsigned > long)powerplay_table + > - > le16_to_cpu(powerplay_table->usNonClockInfoArrayOffset) + > + non_clock_info_array_offset + > > pstate_entry->ucNonClockStateIndex * > > powerplay_table->ucNonClockSize); > > @@ -947,12 +1070,23 @@ int pp_tables_get_entry(struct pp_hwmgr *hwmgr, > pnon_clock_info); > > for (i = 0; i < powerplay_table->ucStateEntrySize-1; i++) { > - const void *pclock_info = (const void *)((unsigned > long)powerplay_table + > - > le16_to_cpu(powerplay_table->usClockInfoArrayOffset) + > + const void *pclock_info; > + > + clock_info_array_offset = > + > le16_to_cpu(powerplay_table->usClockInfoArrayOffset); > + clock_info_offset = clock_info_array_offset + > + pstate_entry->ucClockStateIndices[i] * > + powerplay_table->ucClockInfoSize; > + if (!pp_table_has_space(hwmgr, clock_info_offset, > + > powerplay_table->ucClockInfoSize)) > + return -1; > + > + pclock_info = (const void *)((unsigned > long)powerplay_table + > + clock_info_array_offset + > > pstate_entry->ucClockStateIndices[i] * > > powerplay_table->ucClockInfoSize); > > - int res = func(hwmgr, &ps->hardware, i, pclock_info); > + res = func(hwmgr, &ps->hardware, i, pclock_info); > > if ((0 == result) && (0 != res)) > result = res; [Severity: High] Are the checks on pclock_arrays->ucEntrySize, powerplay_table->ucClockInfoSize, and the DPM level loop counts in pp_tables_get_entry() sufficient to prevent out-of-bounds accesses in the clock info callbacks? Unlike pnon_clock_arrays->ucEntrySize and powerplay_table->ucNonClockSize (which are checked against ATOM_PPLIB_NONCLOCKINFO_VER1 and ATOM_PPLIB_NONCLOCKINFO_VER2), pclock_arrays->ucEntrySize and powerplay_table->ucClockInfoSize are only checked to be non-zero. If either field is between 1 and 8 bytes at the end of hwmgr->soft_pp_table, pp_table_has_space() succeeds, and when func is smu7_get_pp_table_entry_callback_func_v0() in drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c: const ATOM_PPLIB_CI_CLOCK_INFO *visland_clk_info = clock_info; ... engine_clock = visland_clk_info->ucEngineClockHigh << 16 | visland_clk_info->usEngineClockLow; memory_clock = visland_clk_info->ucMemoryClockHigh << 16 | visland_clk_info->usMemoryClockLow; it casts clock_info to const ATOM_PPLIB_CI_CLOCK_INFO * (9 bytes) and reads up to 8 bytes past hwmgr->soft_pp_table. In addition, the DPM level loops in pp_tables_get_entry() iterate i up to pstate_entry_v2->ucNumDPMLevels or powerplay_table->ucStateEntrySize - 1 without bounding i against hwmgr->platform_descriptor.hardwareActivityPerformanceLevels. When func is smu8_dpm_get_pp_table_entry_callback() in drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c: smu8_ps->levels[index].engineClock = table->entries[clock_info_index].clk; smu8_ps->levels[index].vddcIndex = (uint8_t)table->entries[clock_info_index].v; or smu10_dpm_get_pp_table_entry_callback() in drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c: smu10_ps->levels[index].engine_clock = 0; smu10_ps->levels[index].vddc_index = 0; smu10_ps->level = index + 1; can an uncapped index overflow smu8_ps->levels[SMU8_MAX_HARDWARE_POWERLEVELS] or smu10_ps->levels[SMU10_MAX_HARDWARE_POWERLEVELS] (both 8 entries)? [ ... ] > @@ -1705,6 +1888,10 @@ static int pp_tables_initialize(struct pp_hwmgr *hwmgr) > hwmgr->need_pp_table_upload = true; > > powerplay_table = get_powerplay_table(hwmgr); > + PP_ASSERT_WITH_CODE((powerplay_table), > + "Missing PowerPlay Table!", return -1); > + PP_ASSERT_WITH_CODE(pp_table_has_space(hwmgr, 0, > sizeof(*powerplay_table)), > + "Invalid PowerPlay Table!", return -1); > > result = init_powerplay_tables(hwmgr, powerplay_table); > [Severity: High] Does checking only sizeof(*powerplay_table) in pp_tables_initialize() leave the remaining initialization helpers vulnerable to out-of-bounds reads when powerplay_table->usTableSize or subtable offsets exceed hwmgr->soft_pp_table_size? After verifying pp_table_has_space(hwmgr, 0, sizeof(*powerplay_table)), pp_tables_initialize() calls init_thermal_controller(), init_overdrive_limits(), init_clock_voltage_dependency(), init_dpm2_parameters(), and init_phase_shedding_table(). Those helpers check powerplay_table->usTableSize against sizeof(ATOM_PPLIB_POWERPLAYTABLE3), sizeof(ATOM_PPLIB_POWERPLAYTABLE4), or sizeof(ATOM_PPLIB_POWERPLAYTABLE5) without verifying that hwmgr->soft_pp_table_size is large enough for those structures or their subtables: 1. init_thermal_controller() checks powerplay_table->usTableSize >= sizeof(ATOM_PPLIB_POWERPLAYTABLE3) and dereferences powerplay_table3->usFanTableOffset up to ATOM_PPLIB_FANTABLE5 without calling pp_table_has_space(). 2. init_overdrive_limits() -> init_overdrive_limits_V2_1() and init_dpm2_parameters() bypass get_extended_header() and directly dereference usExtendendedHeaderOffset (and usPPMTableOffset in init_dpm2_parameters()) without bounds checks. 3. init_clock_voltage_dependency(), init_dpm2_parameters(), and init_phase_shedding_table() dereference usVddcDependencyOnSCLKOffset, usVddciDependencyOnMCLKOffset, usVddcDependencyOnMCLKOffset, usMaxClockVoltageOnDCOffset, usMvddDependencyOnMCLKOffset, usCACLeakageTableOffset, and usVddcPhaseShedLimitsTableOffset, along with their variable-length entries[] arrays, without checking pp_table_has_space(). ! -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
