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

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

[Taimuraz Kaitmazov]

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.

Ordinary use does not reach it. The chain has been submitted before any of
this runs, so aie2_cmdlist_fill_slot() has already called
amdxdna_cmd_get_op() on that BO and amdxdna_gem_vmap() has cached its
address. What makes it reachable is that the BO is resolved again by
handle here, and the handle is userspace's to recycle: closing it after
submission and importing a dma-buf whose exporter implements no vmap onto
the same id leaves amdxdna_gem_get_obj() returning an object this cannot
map, since prime_import() types every import AMDXDNA_BO_SHARE.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 855da8c79a1c..4e4a9cf5c327 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;
 		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));
-- 
2.55.0

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

[Taimuraz Kaitmazov]

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.

Ordinary use does not reach it. The chain has been submitted before any of
this runs, so aie2_cmdlist_fill_slot() has already called
amdxdna_cmd_get_op() on that BO and amdxdna_gem_vmap() has cached its
address. What makes it reachable is that the BO is resolved again by
handle here, and the handle is userspace's to recycle: closing it after
submission and importing a dma-buf whose exporter implements no vmap onto
the same id leaves amdxdna_gem_get_obj() returning an object this cannot
map, since prime_import() types every import AMDXDNA_BO_SHARE.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 855da8c79a1c..4e4a9cf5c327 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;
 		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));
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Kernel panic via `__counted_by` array bounds violation when userspace supplies `command_count == 0`.
--

--- Patch [1]: [PATCH] accel/amdxdna: put the chained BO when its mapping fails ---
commit 78a13180a3a2715c4d150e03603955519d45a53a
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: put the chained BO when its mapping fails

This commit fixes a reference leak in amdxdna_cmd_set_error(). When the
chained BO fails to map, the previously acquired reference on the BO is now
properly dropped.

> 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 code unconditionally access
cc->data[0] before reaching here?

Looking slightly above this diff hunk in 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);

Because the data flexible array member in struct amdxdna_cmd_chain is
annotated with __counted_by(command_count), accessing cc->data[0] when
command_count is 0 will result in an array bounds violation.

Can a user intentionally trigger a kernel panic and a local denial of service
by submitting a crafted ERT_CMD_CHAIN with command_count == 0 that later times
out or fails?

>  		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));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819230721.287529-1-taimuraz@kaitmazov.com?part=1

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

[sashiko-bot]

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