Re: [PATCH] drm/amdgpu: bound ATOM bytecode cursor within the VBIOS image

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The newly added bounds check in `atom_op_switch` breaks out of the loop on malformed data without setting the abort flag, allowing garbage execution.

Pre-existing issues:
- [Critical] Kernel stack buffer overflow due to unit mismatch between `ps_size` (passed in bytes) and parameter array indexing (processed in dwords).
- [Critical] Nested ATOM command tables can underflow `ps_size` and bypass bounds checks, causing an arbitrary kernel stack buffer overflow.
- [Critical] The bounds check for `ATOM_ARG_FB` scratch memory accesses is vulnerable to an off-by-one error and integer overflow, allowing arbitrary out-of-bounds kernel memory corruption.
- [High] The patch bounds-checks the initial opcode fetch but fails to bound variable-length operand fetches, leaving the interpreter vulnerable to out-of-bounds reads.
- [High] Command table directory index `base` is read from the VBIOS image without bounds checking the index calculation itself.
--

--- Patch [1]: [PATCH] drm/amdgpu: bound ATOM bytecode cursor within the VBIOS image ---
commit e34469f5366554f7ff6d1ad26ef62ec969ed0a40
Author: Hari Mishal <[email protected]>
drm/amdgpu: bound ATOM bytecode cursor within the VBIOS image

The commit bounds-checks several ATOM interpreter opcode fetch and table
jump paths against the VBIOS image size to prevent out-of-bounds reads
on malformed images. It utilizes the existing ctx->bios_size infrastructure.

> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c
> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c

[ ... ]

> @@ -1032,8 +1032,12 @@ static void atom_op_switch(atom_exec_context *ctx, int *ptr, int arg)
>  	uint32_t src, val, target;
>  	SDEBUG("   switch: ");
>  	src = atom_get_src(ctx, attr, ptr);
> -	while (U16(*ptr) != ATOM_CASE_END)
> +	while ((uint32_t)*ptr + 2 <= ctx->ctx->bios_size &&
> +	       U16(*ptr) != ATOM_CASE_END)
>  		if (U8(*ptr) == ATOM_CASE_MAGIC) {
> +			/* magic byte + immediate (up to 4) + 2-byte target */
> +			if ((uint32_t)*ptr + 7 > ctx->ctx->bios_size)
> +				break;

[Severity: High]
Does breaking out of this loop without setting ctx->abort = true allow the
interpreter to continue execution?

It seems the function advances the pointer and returns normally when this
bounds check fails, which might cause the interpreter loop to fetch the next
byte from the middle of the switch data and execute it as garbage opcodes.

[ ... ]

> @@ -1237,6 +1248,13 @@ static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index,
>  	if (!base)
>  		return -EINVAL;
>  
> +	/* the table offset itself comes from the VBIOS master table */
> +	if ((uint32_t)base + ATOM_CT_CODE_PTR >= ctx->bios_size) {

[Severity: High]
This isn't a bug introduced by this patch, but is the retrieval of base itself
bounds-checked before this new check?

The command table directory index is read via:
CU16(ctx->cmd_table + 4 + 2 * index)

If index is arbitrarily large and controlled by the VBIOS, could the macro
perform an out-of-bounds heap read before reaching this newly added validation?

[ ... ]

>  	if (ctx->execute_depth >= ATOM_EXECUTE_MAX_DEPTH) {
>  		DRM_ERROR("atombios command table nesting exceeded limit (%u)\n",
>  			  ATOM_EXECUTE_MAX_DEPTH);

[Severity: Critical]
This isn't a bug introduced by this patch, but are nested ATOM command tables
safe from size underflows?

When atom_op_calltable() recursively calls amdgpu_atom_execute_table_locked(),
it passes ctx->ps_size - ctx->ps_shift for the params_size.

If a malformed VBIOS specifies a ps_shift larger than ps_size, could this
uncontrolled subtraction underflow and promote to a large unsigned bound
during execution, bypassing bounds checks like idx < ctx->ps_size?

[Severity: Critical]
This is another pre-existing issue, but do atom_get_src_int() and
atom_put_dst() incorrectly process ps_size as a dword limit instead of bytes?

External callers pass a stack-allocated structure and its byte size to
amdgpu_atom_execute_table(). If the VBIOS-supplied idx is used to index the
uint32_t *ps array, could passing idx = 7 for an 8-byte structure pass the
idx < 8 check but access the 7th dword, leading to out-of-bounds stack
reads and writes?

[ ... ]

> @@ -1273,6 +1291,13 @@ static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index,
>  
>  	debug_depth++;
>  	while (1) {
> +		/* jump/skip targets are bytecode-supplied; stay inside the image */
> +		if ((uint32_t)ptr >= ctx->bios_size) {
> +			DRM_ERROR("atombios cursor 0x%x outside VBIOS image (table %04X)\n",
> +				  ptr, base);
> +			ret = -EINVAL;
> +			goto free;
> +		}
>  		op = CU8(ptr++);

[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check only
protect the opcode byte itself?

The individual opcode handlers executed later via opcode_table[op].func()
appear to unconditionally fetch subsequent variable-length arguments. If the
image ends immediately after an opcode, could these operand fetches perform
out-of-bounds reads?

[Severity: Critical]
This is a pre-existing issue, but is the bounds check for ATOM_ARG_FB scratch
memory accesses in atom_put_dst() susceptible to integer overflow?

The bounds check appears to be:
if ((gctx->fb_base + (idx * 4)) > gctx->scratch_size_bytes)

Since it uses > instead of >=, it might allow an off-by-one access. More
importantly, if gctx->fb_base and idx are 32-bit values controlled by the
VBIOS, could their addition wrap around a 32-bit integer and bypass the
bounds check entirely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1