Re: [PATCH] accel/amdxdna: put the chained BO when its mapping fails

[sashiko-bot@kernel.org]

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 <taimuraz@kaitmazov.com>

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/20260819230852.287751-1-taimuraz@kaitmazov.com?part=1