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
