Re: [PATCH AUTOSEL 6.18] drm/amdgpu: cap ATOM command table nesting depth

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Out-of-bounds stack/heap access due to unvalidated `ps_shift` causing negative `ps_size`.
- [Critical] Integer overflow in `ATOM_ARG_FB` bounds check allows out-of-bounds memory corruption.
- [Medium] The command table loop timeout mechanism in `atom_op_jump` can be trivially defeated by a multi-jump loop.
- [Low] The `debug_depth` counter leaks permanently when the execution of a command table is aborted.
--

commit 0c77b7dbed85cb54e1624a76d5ddd1e7608f6557
Author: Candice Li <candice.li@amd.com>

drm/amdgpu: cap ATOM command table nesting depth

Cap nesting at 32 levels with execute_depth and return -ELOOP when exceeded.

> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c
> index 7a063e44d4298..639649ef6dd03 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
> @@ -59,6 +59,9 @@
>  
>  #define ATOM_CMD_TIMEOUT_SEC	20
>  
> +/* Limit ATOM command table recursion (calltable) to avoid kernel stack overflow. */
> +#define ATOM_EXECUTE_MAX_DEPTH	32
> +

[Severity: Critical]
This is a pre-existing issue, but can this lead to an out-of-bounds access?

In atom_op_calltable(), the size calculation could evaluate to a negative
integer:

drivers/gpu/drm/amd/amdgpu/atom.c:atom_op_calltable() {
    ...
    r = amdgpu_atom_execute_table_locked(ctx->ctx, idx, ctx->ps + ctx->ps_shift,
                                         ctx->ps_size - ctx->ps_shift);
    ...
}

If a crafted VBIOS provides a large ps_shift value, ctx->ps_size - ctx->ps_shift
will be negative. When passed as params_size into
amdgpu_atom_execute_table_locked(), this negative value propagates.

Later, in atom_put_dst(), this negative size is promoted to a large unsigned
integer, which would bypass the bounds check:

drivers/gpu/drm/amd/amdgpu/atom.c:atom_put_dst() {
    ...
    if (idx >= ctx->ps_size) {
    ...
}

Could this be leveraged for out-of-bounds reads or writes on the stack or heap?

[Severity: Critical]
This is a pre-existing issue, but does this bounds check have an integer
overflow vulnerability?

If a crafted VBIOS sets fb_base to a value near UINT32_MAX, the addition in
the ATOM_ARG_FB bounds check can overflow:

drivers/gpu/drm/amd/amdgpu/atom.c:atom_put_dst() {
    ...
    if ((gctx->fb_base + (idx * 4)) > gctx->scratch_size_bytes) {
    ...
}

If gctx->fb_base + (idx * 4) overflows into a small positive number, it passes
the bounds check. The subsequent memory access uses the original huge fb_base:

drivers/gpu/drm/amd/amdgpu/atom.c:atom_put_dst() {
    ...
    gctx->scratch[(gctx->fb_base / 4) + idx] = val;
    ...
}

Can this lead to out-of-bounds memory corruption?

[Severity: Medium]
This is a pre-existing issue, but can the command table loop timeout be easily
defeated?

The timeout mechanism in atom_op_jump() resets the tracker when the script jumps
to a different instruction:

drivers/gpu/drm/amd/amdgpu/atom.c:atom_op_jump() {
    ...
    } else {
        ctx->last_jump = ctx->start + target;
        ctx->last_jump_jiffies = jiffies;
    }
    ...
}

If a VBIOS script uses multiple jump instructions in a loop (for example,
jumping between A and B), ctx->last_jump == (ctx->start + target) will always
be false. This forces the execution into the else block, continuously resetting
the 20-second timeout tracker.

Could this allow a multi-jump loop to hang the driver indefinitely, causing a
denial of service?

[ ... ]

> @@ -1285,6 +1295,7 @@ static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index,
>  free:
>  	if (ws)
>  		kfree(ectx.ws);
> +	ctx->execute_depth--;

[Severity: Low]
This is a pre-existing issue, but does the debug_depth counter leak when an
execution is aborted?

In amdgpu_atom_execute_table_locked(), if ectx.abort becomes true, the code
jumps directly to the free label:

    if (ectx.abort) {
        ...
        goto free;
    }

Because the free label is located after the debug_depth--; statement, the
counter is never decremented on this error path. When amdgpu_atom_debug is
enabled, could this cause unbounded indentation in debug logs over time?

>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-274-sashal@kernel.org?part=1