Re: [PATCH AUTOSEL 6.18-5.15] drm/amd/pm: Guard VBIOS AC timing table walk

[sashiko-bot@kernel.org]

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 <xiang.liu@amd.com>

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 = &reg_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/20261019135441.1420968-423-sashal@kernel.org?part=1