From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Validate command buffer payload count Date: Mon, 23 Feb 2026 06:27:49 +1000 Message-ID: In-Reply-To: <20260219211946.1920485-1-lizhi.hou@amd.com> References: <20260219211946.1920485-1-lizhi.hou@amd.com> <20260219211946.1920485-1-lizhi.hou@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The fix is correct and the commit message clearly explains what is being va= lidated and why. > + if (unlikely(count <=3D num_masks || > + count * sizeof(u32) + > + offsetof(struct amdxdna_cmd, data[0]) > > + abo->mem.size)) { The arithmetic checks out. `count` comes from `FIELD_GET(AMDXDNA_CMD_COUNT,= cmd->header)` where `AMDXDNA_CMD_COUNT` is `GENMASK(22, 12)`, giving a max= imum value of 2047. Since `sizeof(u32)` is `size_t`, the multiplication pro= motes `count` to `size_t` and the maximum result is 8188 + 4 =3D 8192 =E2= =80=94 no overflow risk. The comparison against `abo->mem.size` (also `size= _t`) is straightforward. Minor note: `offsetof(struct amdxdna_cmd, data[0])` is equivalent to `offse= tof(struct amdxdna_cmd, data)`. The `[0]` subscript in `offsetof` is a GNU = extension that the kernel uses elsewhere, so this is fine, just slightly un= usual. One observation about pre-existing caller patterns (not introduced by this = patch): some callers like `aie2_init_exec_cu_req()` do not check the return= value for NULL before using the pointer: ```c cmd =3D amdxdna_cmd_get_payload(cmd_bo, &cmd_len); if (cmd_len > sizeof(cu_req->payload)) return -EINVAL; ... memcpy(cu_req->payload, cmd, cmd_len); ``` When the new validation triggers, `cmd` is NULL and `cmd_len` is 0, so the = `memcpy` becomes `memcpy(dst, NULL, 0)`. In practice this is harmless in th= e kernel, but it is technically undefined behavior per the C standard. Othe= r callers like the `ERT_CMD_CHAIN` path at line 972 do properly check `!pay= load`. This is a pre-existing pattern and not a problem introduced by this = patch, but a follow-up to add explicit NULL checks in callers would be wort= hwhile. No other issues found. The patch is a clean, correct fix for a real validat= ion gap. --- Generated by Claude Code Patch Reviewer