On Thu, Jul 30, 2026 at 1:33 PM <[email protected]> wrote:
>
> 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.
>
Hari, can you double check this and respin if necessary?
Thanks,
Alex
> [ ... ]
>
> > @@ -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
