Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Asynchronous GEM handle resolution in the error path allows userspace to overwrite arbitrary GEM objects. - [High] The `__counted_by` annotation on `struct amdxdna_cmd_chain` resides in shared memory, allowing userspace to trigger a kernel panic via concurrent modification. - [Medium] State modification occurs before error payload is written, causing a TOCTOU race with user space. -- --- Patch [1]: [PATCH] accel/amdxdna: put the chained BO when its mapping fails --- commit f44c3e00164e40d1dcd2962edb86e83404245c37 Author: Taimuraz Kaitmazov <[email protected]> accel/amdxdna: put the chained BO when its mapping fails amdxdna_cmd_set_error() looks up the first BO of a command chain, which takes a reference, and drops it at the end of the function. The mapping of that BO is established in between, and the failure path returns without the put, so the reference is leaked. > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c > b/drivers/accel/amdxdna/amdxdna_ctx.c > index 31a414c3f0d96..888e857ec5582 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c > @@ -183,8 +183,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo, > if (!abo) > return -EINVAL; [Severity: Critical] This is a pre-existing issue, but does this asynchronous GEM handle resolution allow userspace to overwrite arbitrary GEM objects? In amdxdna_cmd_set_error(), the inner command buffer is resolved dynamically by reading a handle ID from cc->data[0] (in shared memory) and looking it up in the client's file descriptor table. Because userspace can concurrently close the original handle after submission and import an arbitrary AMDXDNA_BO_SHARE dmabuf (such as a read-only shared dmabuf from another process) to the same handle ID, the lookup could resolve to a victim object instead. The driver maps it and blindly overwrites up to its size with 0xff and error data. Can a malicious process use this to corrupt memory across process boundaries? [Severity: High] This is a pre-existing issue, but can userspace trigger a kernel panic via concurrent modification of the command_count? struct amdxdna_cmd_chain defines a flexible array annotated with __counted_by(command_count). Because this struct is stored in AMDXDNA_BO_SHARE shared memory, userspace can modify command_count concurrently. When the kernel accesses data[0] in amdxdna_cmd_set_error() (or data[i] in aie2_cmdlist_multi_execbuf()), the compiler-generated bounds check reads the maliciously modified command_count. If modified to 0, the check fails, and the kernel panics. > cmd = amdxdna_gem_vmap(abo); > - if (!cmd) > + if (!cmd) { > + amdxdna_gem_put_obj(abo); > return -ENOMEM; > + } > } > > memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd)); [Severity: Medium] This is a pre-existing issue, but is there a time-of-check to time-of-use race with user space here? Earlier in amdxdna_cmd_set_error(), the command header's state is modified to error_state: cmd->header &= ~AMDXDNA_CMD_STATE; cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); Since this happens before the inner payload or error data is written via memset() below, and the header is in a shared memory AMDXDNA_BO_SHARE object, could userspace polling for completion observe the error state immediately? It seems userspace could then proceed to read the error payload data before the kernel has completed mapping the inner object and writing the error data, resulting in reading uninitialized, stale, or partially updated error payloads. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
