Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/amd/pm: unchecked index table walk in atomctrl_set_mc_reg_address_table() --
commit a6c602ed4f05fc0e3395f511194128fee6bac1cc Author: Xiang Liu <[email protected]> drm/amd/pm: Guard VBIOS AC timing table walk This commit rejects AC timing blocks with a stride smaller than a dword and bounds the data-block and END marker reads in atomctrl_retrieve_ac_timing() by the returned VRAM_Info table size. > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c > b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c > index ce166a7f8e420..85bf1527c6f44 100644 > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c [ ... ] > @@ -157,6 +170,7 @@ int atomctrl_initialize_mc_reg_table( > } > > if (0 == result) { > + table_end = (uint8_t *)vram_info + size; > reg_block = (ATOM_INIT_REG_BLOCK *) > ((uint8_t *)vram_info + > le16_to_cpu(vram_info->usMemClkPatchTblOffset)); > result = atomctrl_set_mc_reg_address_table(reg_block, table); > } > > if (0 == result) { > result = atomctrl_retrieve_ac_timing(module_index, > - reg_block, table); > + reg_block, table_end, table); > } [Severity: Medium] The commit message notes that the reads are bounded by the returned VRAM_Info table size so malformed index/data sizes do not push the timing walk past the table. However, in both atomctrl_initialize_mc_reg_table() and atomctrl_initialize_mc_reg_table_v2_2(), table_end is only passed to atomctrl_retrieve_ac_timing(). Prior to that call, reg_block is computed from vram_info->usMemClkPatchTblOffset and passed directly to atomctrl_set_mc_reg_address_table() without checking whether reg_block or its index table fits within table_end: drivers/gpu/drm/amd/pm/powerplay/hwmgr/ppatomctrl.c:atomctrl_set_mc_reg_address_table() { uint8_t i = 0; uint8_t num_entries = (uint8_t)((le16_to_cpu(reg_block->usRegIndexTblSize)) / sizeof(ATOM_INIT_REG_INDEX_FORMAT)); ATOM_INIT_REG_INDEX_FORMAT *format = ®_block->asRegIndexBuf[0]; num_entries--; /* subtract 1 data end mark entry */ PP_ASSERT_WITH_CODE((num_entries <= VBIOS_MC_REGISTER_ARRAY_SIZE), "Invalid VramInfo table.", return -1); /* ucPreRegDataLength bit6 = 1 is the end of memory clock AC timing registers */ while ((!(format->ucPreRegDataLength & ACCESS_PLACEHOLDER)) && (i < num_entries)) { ... } In atomctrl_set_mc_reg_address_table(), there are a few issues when handling a malformed VBIOS table: First, reg_block->usRegIndexTblSize is dereferenced and format->asRegIndexBuf is walked without validating against table_end. Second, the while loop condition evaluates format->ucPreRegDataLength before checking i < num_entries, which reads one entry past num_entries when ACCESS_PLACEHOLDER is not encountered earlier. Third, casting le16_to_cpu(reg_block->usRegIndexTblSize) / sizeof(ATOM_INIT_REG_INDEX_FORMAT) to uint8_t truncates the 16-bit entry count before decrementing num_entries and comparing against VBIOS_MC_REGISTER_ARRAY_SIZE. Should reg_block and atomctrl_set_mc_reg_address_table() also be bounded by table_end, and should i < num_entries be checked before dereferencing format? > > return result; [ ... ] > @@ -198,6 +213,7 @@ int atomctrl_initialize_mc_reg_table_v2_2( > } > > if (0 == result) { > + table_end = (uint8_t *)vram_info + size; > reg_block = (ATOM_INIT_REG_BLOCK *) > ((uint8_t *)vram_info + > le16_to_cpu(vram_info->usMemClkPatchTblOffset)); > result = atomctrl_set_mc_reg_address_table(reg_block, table); > } > > if (0 == result) { > result = atomctrl_retrieve_ac_timing(module_index, > - reg_block, table); > + reg_block, table_end, table); > } > > return result; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
