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

On Thu, Jul 30, 2026 at 1:33=E2=80=AFPM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 6 potential issu=
e(s) to consider:
>
> New issues:
> - [High] The newly added bounds check in `atom_op_switch` breaks out of t=
he 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 `p=
s_size` (passed in bytes) and parameter array indexing (processed in dwords=
).
> - [Critical] Nested ATOM command tables can underflow `ps_size` and bypas=
s bounds checks, causing an arbitrary kernel stack buffer overflow.
> - [Critical] The bounds check for `ATOM_ARG_FB` scratch memory accesses i=
s vulnerable to an off-by-one error and integer overflow, allowing arbitrar=
y out-of-bounds kernel memory corruption.
> - [High] The patch bounds-checks the initial opcode fetch but fails to bo=
und variable-length operand fetches, leaving the interpreter vulnerable to =
out-of-bounds reads.
> - [High] Command table directory index `base` is read from the VBIOS imag=
e 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 infrastructu=
re.
>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/am=
dgpu/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 *ct=
x, int *ptr, int arg)
> >       uint32_t src, val, target;
> >       SDEBUG("   switch: ");
> >       src =3D atom_get_src(ctx, attr, ptr);
> > -     while (U16(*ptr) !=3D ATOM_CASE_END)
> > +     while ((uint32_t)*ptr + 2 <=3D ctx->ctx->bios_size &&
> > +            U16(*ptr) !=3D ATOM_CASE_END)
> >               if (U8(*ptr) =3D=3D ATOM_CASE_MAGIC) {
> > +                     /* magic byte + immediate (up to 4) + 2-byte targ=
et */
> > +                     if ((uint32_t)*ptr + 7 > ctx->ctx->bios_size)
> > +                             break;
>
> [Severity: High]
> Does breaking out of this loop without setting ctx->abort =3D 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 n=
ext
> 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(stru=
ct 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 >=3D ctx->bios_size) {
>
> [Severity: High]
> This isn't a bug introduced by this patch, but is the retrieval of base i=
tself
> 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 macr=
o
> perform an out-of-bounds heap read before reaching this newly added valid=
ation?
>
> [ ... ]
>
> >       if (ctx->execute_depth >=3D 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 ta=
bles
> safe from size underflows?
>
> When atom_op_calltable() recursively calls amdgpu_atom_execute_table_lock=
ed(),
> 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 by=
tes?
>
> 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 t=
he
> uint32_t *ps array, could passing idx =3D 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(stru=
ct atom_context *ctx, int index,
> >
> >       debug_depth++;
> >       while (1) {
> > +             /* jump/skip targets are bytecode-supplied; stay inside t=
he image */
> > +             if ((uint32_t)ptr >=3D ctx->bios_size) {
> > +                     DRM_ERROR("atombios cursor 0x%x outside VBIOS ima=
ge (table %04X)\n",
> > +                               ptr, base);
> > +                     ret =3D -EINVAL;
> > +                     goto free;
> > +             }
> >               op =3D CU8(ptr++);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this bounds check onl=
y
> 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 perfo=
rm
> out-of-bounds reads?
>
> [Severity: Critical]
> This is a pre-existing issue, but is the bounds check for ATOM_ARG_FB scr=
atch
> 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 >=3D, it might allow an off-by-one access. Mor=
e
> 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 =C2=B7 https://sashiko.dev/#/patchset/20260730170011.28=
[email protected]?part=3D1