Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Reference count leak of a GEM object on an error path in `amdxdna_cmd_set_error()`. - [High] Calling `memcpy` with a NULL source pointer when processing empty payloads. - [High] Missing READ_ONCE() on shared memory reads allows compiler reloads, causing a Time-of-Check to Time-of-Use (TOCTOU) vulnerability that bypasses bounds checks. -- --- Patch [1]: [PATCH V1] accel/amdxdna: Remove __counted_by from struct amdxdna_cmd_chain --- commit fcdbc0a8d70945b3b3d7614f4b69f29895cd2af9 Author: Lizhi Hou <[email protected]> accel/amdxdna: Remove __counted_by from struct amdxdna_cmd_chain The commit removes the __counted_by annotation from the data array in struct amdxdna_cmd_chain. This prevents kernel panics caused by bounds checks failing if userspace concurrently modifies command_count to zero in shared memory. > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h > b/drivers/accel/amdxdna/amdxdna_ctx.h > index b6bef3af7dab4..6e78bab8a02c0 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.h > +++ b/drivers/accel/amdxdna/amdxdna_ctx.h > @@ -55,7 +55,7 @@ struct amdxdna_cmd_chain { > u32 submit_index; > u32 error_index; > u32 reserved[3]; > - u64 data[] __counted_by(command_count); > + u64 data[]; > }; [Severity: High] This is a pre-existing issue, but since this patch addresses concurrent userspace modifications to command_count, are there other Time-of-Check to Time-of-Use (TOCTOU) issues with this field? Looking at drivers/accel/amdxdna/aie2_message.c:aie2_cmdlist_multi_execbuf(): ccnt = payload->command_count; if (payload_len < struct_size(payload, data, ccnt)) { XDNA_DBG(xdna, "Invalid command count %d", ccnt); return -EINVAL; } op = ERT_INVALID_CMD; for (i = 0; i < ccnt; i++) { u32 boh = (u32)(payload->data[i]); Can the compiler reload payload->command_count inside the loop condition since READ_ONCE() is not used? If userspace changes the value to a massive number after the payload_len check passes, could this bypass the bounds check and cause out-of-bounds reads on adjacent kernel memory? A similar issue appears in drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_cmd_get_payload() where cmd->header is read multiple times: if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) num_masks = 0; else num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header); if (size) { count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header); if (unlikely(count <= num_masks || count * sizeof(u32) + offsetof(struct amdxdna_cmd, data[0]) > abo->mem.size)) { *size = 0; return NULL; } *size = (count - num_masks) * sizeof(u32); } Could a compiler reload cause the calculated size to exceed the BO size? [Severity: High] This is a pre-existing issue, but does amdxdna_cmd_get_payload() return NULL if the payload is empty? In drivers/accel/amdxdna/aie2_message.c:aie2_init_exec_cu_req(): cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len); if (cmd_len > sizeof(cu_req->payload)) return -EINVAL; cu_req->cu_idx = amdxdna_cmd_get_cu_idx(cmd_bo); if (cu_req->cu_idx == INVALID_CU_IDX) return -EINVAL; memcpy(cu_req->payload, cmd, cmd_len); If userspace constructs a command BO where count <= num_masks, amdxdna_cmd_get_payload() sets cmd_len to 0 and returns NULL. If a valid CU index is still returned, this calls memcpy(cu_req->payload, NULL, 0). Could this undefined behavior reliably crash the kernel if the UBSAN bounds checker with panic_on_warn is enabled? The same pattern seems to exist in aie2_cmdlist_fill_cf() and aie2_cmdlist_fill_npu_cf(). [Severity: High] This is a pre-existing issue, but is there a reference leak on the error path in amdxdna_cmd_set_error()? In drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_cmd_set_error(): if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) { cc = amdxdna_cmd_get_payload(abo, NULL); cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0; abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE); if (!abo) return -EINVAL; cmd = amdxdna_gem_vmap(abo); if (!cmd) return -ENOMEM; } If amdxdna_gem_vmap(abo) fails, the function returns -ENOMEM early. Should there be an amdxdna_gem_put_obj(abo) call to avoid leaking the GEM object reference acquired by amdxdna_gem_get_obj()? Could a malicious userspace repeatedly supply a BO handle that fails to map to cause unfreeable memory? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
