[PATCH 0/2] accel/amdxdna: make the shared command header safe to read

[PATCH 0/2] accel/amdxdna: make the shared command header safe to read

[Taimuraz Kaitmazov]

The command header lives in a BO user space keeps mapped, and the driver both
re-reads it on every accessor call and read-modify-writes it in place.

Sent as a series rather than two standalone patches: they touch the same
function and do not apply independently.

Taimuraz Kaitmazov (2):
  accel/amdxdna: check the command chain payload before using it
  accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

 drivers/accel/amdxdna/amdxdna_ctx.c | 65 +++++++++++++++++++++--------
 drivers/accel/amdxdna/amdxdna_ctx.h | 13 ++++--
 2 files changed, 57 insertions(+), 21 deletions(-)

-- 
2.55.0

[PATCH 1/2] accel/amdxdna: check the command chain payload before using it

[Taimuraz Kaitmazov]

amdxdna_cmd_get_payload() only bounds-checks the payload when a size
pointer is passed, so its two callers get different guarantees from one
function. amdxdna_cmd_set_error() takes the unchecked form and then reads
cc->command_count and cc->data[0], neither of which has been shown to
lie inside the BO.

Make the size mandatory and add amdxdna_cmd_get_chain(), which returns
the chain only once the declared command count is known to fit.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 51 ++++++++++++++++++++++-------
 drivers/accel/amdxdna/amdxdna_ctx.h |  2 ++
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 855da8c79a1c..143cbbbf4b31 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -125,20 +125,42 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	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);
+	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);
+
 	return &cmd->data[num_masks];
 }
 
+/*
+ * Returns the chain payload of @abo, with @count set to a command count that
+ * has been checked to fit. The chain fields live in a BO user space keeps
+ * mapped, so nothing may read them without going through here.
+ */
+struct amdxdna_cmd_chain *
+amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count)
+{
+	struct amdxdna_cmd_chain *cc;
+	u32 len, ccnt;
+
+	cc = amdxdna_cmd_get_payload(abo, &len);
+	if (!cc || len < sizeof(*cc))
+		return NULL;
+
+	ccnt = cc->command_count;
+	if (len < struct_size(cc, data, ccnt))
+		return NULL;
+
+	*count = ccnt;
+	return cc;
+}
+
 u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
 {
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
@@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
 
 	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;
+		u32 ccnt;
+
+		cc = amdxdna_cmd_get_chain(abo, &ccnt);
+		if (!cc || !ccnt)
+			return -EINVAL;
+
+		cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0;
 		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
 		if (!abo)
 			return -EINVAL;
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
index b6bef3af7dab..f6529d512217 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.h
+++ b/drivers/accel/amdxdna/amdxdna_ctx.h
@@ -196,6 +196,8 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
 }
 
 void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);
+struct amdxdna_cmd_chain *
+amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count);
 u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo);
 int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 			  struct amdxdna_sched_job *job, u32 cmd_idx,
-- 
2.55.0

Re: [PATCH 1/2] accel/amdxdna: check the command chain payload before using it

[sashiko-bot]

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

New issues:
- [Medium] Missing READ_ONCE() when reading `cc->command_count` from a user-space mapped buffer introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability.

Pre-existing issues:
- [High] Reference leak for the chained GEM object `abo` in `amdxdna_cmd_set_error()` when `amdxdna_gem_vmap()` fails.
- [High] Unprotected access to user-mapped flexible array `cc->data[0]` causes data races and triggers UBSAN panics via `__counted_by`.
--

commit a1b05572fd8c69d4959c5969b2a34b2e5ce07591
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
Subject: accel/amdxdna: check the command chain payload before using it

This commit introduces a new function amdxdna_cmd_get_chain() to ensure the
chain payload of a BO is checked before use. It makes the size parameter
mandatory in amdxdna_cmd_get_payload() and updates amdxdna_cmd_set_error()
to utilize the new chain validation logic.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 855da8c79a1cd..143cbbbf4b310 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c

> [ ... ]

> @@ -125,20 +125,42 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)

> [ ... ]

> +/*
> + * Returns the chain payload of @abo, with @count set to a command count that
> + * has been checked to fit. The chain fields live in a BO user space keeps
> + * mapped, so nothing may read them without going through here.
> + */
> +struct amdxdna_cmd_chain *
> +amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count)
> +{
> +	struct amdxdna_cmd_chain *cc;
> +	u32 len, ccnt;
> +
> +	cc = amdxdna_cmd_get_payload(abo, &len);
> +	if (!cc || len < sizeof(*cc))
> +		return NULL;
> +
> +	ccnt = cc->command_count;
> +	if (len < struct_size(cc, data, ccnt))
> +		return NULL;
> +
> +	*count = ccnt;

[Severity: Medium]
Since the memory for cc resides in a GEM object mapped to userspace, can the
compiler perform multiple memory loads of cc->command_count here because it
lacks a READ_ONCE() wrapper?

If a user modifies cc->command_count concurrently between the struct_size()
bounds check and the assignment to *count, could this bypass the newly added
bounds check and pass a larger ccnt to the caller?

> +	return cc;
> +}

> [ ... ]

> @@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>  	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>  
>  	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;
> +		u32 ccnt;
> +
> +		cc = amdxdna_cmd_get_chain(abo, &ccnt);
> +		if (!cc || !ccnt)
> +			return -EINVAL;
> +
> +		cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0;
>  		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);

[Severity: High]
This is a pre-existing issue, but since struct amdxdna_cmd_chain defines data
as a flexible array u64 data[] __counted_by(command_count), does accessing
cc->data[0] dynamically re-read cc->command_count from memory to assert
0 < command_count?

If cc is backed by user-mapped memory, couldn't a malicious user concurrently
write 0 to command_count, causing the dynamically validated access to fail
and trigger a UBSAN kernel panic if CONFIG_UBSAN_BOUNDS is enabled?

>  		if (!abo)
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this function leak the reference to
the chained GEM object abo if amdxdna_gem_vmap() fails?

Looking at amdxdna_cmd_set_error(), it successfully looks up the chained BO
using amdxdna_gem_get_obj(), which increments its reference count:

	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() fails, the function returns -ENOMEM without calling
amdxdna_gem_put_obj(abo). Can an unprivileged user trigger this by causing
job failures under memory pressure or by crafting a BO that fails to map,
leading to a memory leak and eventual Denial of Service?

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

[PATCH 2/2] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[Taimuraz Kaitmazov]

struct amdxdna_cmd::header packs STATE, OPCODE, COUNT and EXTRA_CU_MASK
into one u32 that lives in a BO user space keeps mapped. The driver
re-reads it on every accessor call and read-modify-writes STATE in place,
with plain accesses that let the compiler split or refetch either side.

Annotate them, as the UMQ ring indices already are. This does not make
the update atomic against user space, it only stops the compiler from
making it worse.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++-----
 drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 143cbbbf4b31..f809bec425a5 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -123,9 +123,10 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	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);
+		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
+					  READ_ONCE(cmd->header));
 
-	count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
+	count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));
 	if (unlikely(count <= num_masks ||
 		     count * sizeof(u32) +
 		     offsetof(struct amdxdna_cmd, data[0]) >
@@ -173,7 +174,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
 		return INVALID_CU_IDX;
 
-	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
+	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header));
 	cu_mask = cmd->data;
 	for (i = 0; i < num_masks; i++) {
 		if (cu_mask[i])
@@ -191,12 +192,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 	struct amdxdna_client *client = job->hwctx->client;
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
 	struct amdxdna_cmd_chain *cc = NULL;
+	u32 header;
 
 	if (!cmd)
 		return -ENOMEM;
 
-	cmd->header &= ~AMDXDNA_CMD_STATE;
-	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
+	header = READ_ONCE(cmd->header);
+	header &= ~AMDXDNA_CMD_STATE;
+	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
+	WRITE_ONCE(cmd->header, header);
 
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
 		u32 ccnt;
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
index f6529d512217..48e1fcfd818f 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.h
+++ b/drivers/accel/amdxdna/amdxdna_ctx.h
@@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)
 	if (!cmd)
 		return ERT_INVALID_CMD;
 
-	return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header);
+	return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header));
 }
 
 static inline void
 amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum ert_cmd_state s)
 {
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
+	u32 header;
 
 	if (!cmd)
 		return;
 
-	cmd->header &= ~AMDXDNA_CMD_STATE;
-	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
+	header = READ_ONCE(cmd->header);
+	header &= ~AMDXDNA_CMD_STATE;
+	header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
+	WRITE_ONCE(cmd->header, header);
 }
 
 static inline enum ert_cmd_state
@@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
 	if (!cmd)
 		return ERT_CMD_STATE_INVALID;
 
-	return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header);
+	return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header));
 }
 
 void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);
-- 
2.55.0

Re: [PATCH 2/2] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing READ_ONCE on shared BO field `command_count` leaves it vulnerable to compiler refetching optimization.
- [High] Memory leak of a GEM object reference on the error path in `amdxdna_cmd_set_error()`.
- [Medium] Race condition where the error state is written to the command header before the error payload is populated.
--

commit b8c13d2d8e5350b458948c5624a0d76772f2851a
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

The driver modifies state flags in place, allowing the compiler to split or
refetch accesses. This patch annotates these accesses with READ_ONCE and
WRITE_ONCE to stop the compiler from optimizing them in unsafe ways.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 143cbbbf4b310..f809bec425a57 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c

[ ... ]

> @@ -191,12 +192,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>  	struct amdxdna_client *client = job->hwctx->client;
>  	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>  	struct amdxdna_cmd_chain *cc = NULL;
> +	u32 header;
>  
>  	if (!cmd)
>  		return -ENOMEM;
>  
> -	cmd->header &= ~AMDXDNA_CMD_STATE;
> -	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	header = READ_ONCE(cmd->header);
> +	header &= ~AMDXDNA_CMD_STATE;
> +	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	WRITE_ONCE(cmd->header, header);

[Severity: Medium]
This is a pre-existing issue, but does writing the error state here create a
race condition where user-space might read stale or garbage data?

The driver modifies the state flags in the shared cmd->header prior to
issuing the memset and memcpy to populate the error payload later in the
function:

	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));
	if (err_data)
		memcpy(cmd->data, err_data, min(size, abo->mem.size - sizeof(*cmd)));

If user-space is polling the state field, could it see the error state and
immediately read the payload before the driver has finished writing it?

[Severity: High]
This is also a pre-existing issue in this same function, but does this code
leak a GEM object reference on the error path?

If amdxdna_gem_vmap() fails after acquiring the abo reference, the code
returns -ENOMEM without calling amdxdna_gem_put_obj():

	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
		...
		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;
	}

Could successive failures eventually exhaust kernel memory resources?

[Severity: High]
This isn't a bug introduced by this patch, but while annotating BO memory
accesses with READ_ONCE, are there other places where shared BO fields are
read without it?

For instance, command_count is read from shared memory and used in bounds
checks in aie2_cmdlist_multi_execbuf():

	ccnt = payload->command_count;
	if (payload_len < struct_size(payload, data, ccnt)) {
		...
		for (i = 0; i < ccnt; i++)

And similarly in amdxdna_cmd_get_chain():

	ccnt = cc->command_count;
	if (len < struct_size(cc, data, ccnt))

Could the compiler refetch command_count from shared memory after the bounds
check?

If user space concurrently modifies the value after the bounds check, could
the subsequent loop read out-of-bounds kernel memory, potentially leading to
a kernel page fault?

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

[PATCH v2 0/2] accel/amdxdna: make the shared command header safe to read

[Taimuraz Kaitmazov]

The command header lives in a BO user space keeps mapped, and the driver both
re-reads it on every accessor call and read-modify-writes it in place.

Sent as a series rather than two standalone patches: they touch the same
function and do not apply independently.

v2: READ_ONCE() on cc->command_count in amdxdna_cmd_get_chain().

Taimuraz Kaitmazov (2):
  accel/amdxdna: check the command chain payload before using it
  accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

 drivers/accel/amdxdna/amdxdna_ctx.c | 65 +++++++++++++++++++++--------
 drivers/accel/amdxdna/amdxdna_ctx.h | 13 ++++--
 2 files changed, 57 insertions(+), 21 deletions(-)

-- 
2.55.0

[PATCH v2 1/2] accel/amdxdna: check the command chain payload before using it

[Taimuraz Kaitmazov]

amdxdna_cmd_get_payload() only bounds-checks the payload when a size
pointer is passed, so its two callers get different guarantees from one
function. amdxdna_cmd_set_error() takes the unchecked form and then reads
cc->command_count and cc->data[0], neither of which has been shown to
lie inside the BO.

Make the size mandatory and add amdxdna_cmd_get_chain(), which returns
the chain only once the declared command count is known to fit.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 51 ++++++++++++++++++++++-------
 drivers/accel/amdxdna/amdxdna_ctx.h |  2 ++
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 855da8c79a1c..9f44e3918bc1 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -125,20 +125,42 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	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);
+	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);
+
 	return &cmd->data[num_masks];
 }
 
+/*
+ * Returns the chain payload of @abo, with @count set to a command count that
+ * has been checked to fit. The chain fields live in a BO user space keeps
+ * mapped, so nothing may read them without going through here.
+ */
+struct amdxdna_cmd_chain *
+amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count)
+{
+	struct amdxdna_cmd_chain *cc;
+	u32 len, ccnt;
+
+	cc = amdxdna_cmd_get_payload(abo, &len);
+	if (!cc || len < sizeof(*cc))
+		return NULL;
+
+	ccnt = READ_ONCE(cc->command_count);
+	if (len < struct_size(cc, data, ccnt))
+		return NULL;
+
+	*count = ccnt;
+	return cc;
+}
+
 u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
 {
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
@@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
 
 	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;
+		u32 ccnt;
+
+		cc = amdxdna_cmd_get_chain(abo, &ccnt);
+		if (!cc || !ccnt)
+			return -EINVAL;
+
+		cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0;
 		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
 		if (!abo)
 			return -EINVAL;
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
index b6bef3af7dab..f6529d512217 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.h
+++ b/drivers/accel/amdxdna/amdxdna_ctx.h
@@ -196,6 +196,8 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
 }
 
 void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);
+struct amdxdna_cmd_chain *
+amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count);
 u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo);
 int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 			  struct amdxdna_sched_job *job, u32 cmd_idx,
-- 
2.55.0

Re: [PATCH v2 1/2] accel/amdxdna: check the command chain payload before using it

[sashiko-bot]

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

Pre-existing issues:
- [High] amdxdna_cmd_set_error() writes the error state to the wrong chain command BO by hardcoding index 0.
--

commit 5dc0906b3cc41016d562034c7287f924681a97d7
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: check the command chain payload before using it

This commit updates amdxdna_cmd_get_payload() to make the size parameter
mandatory and introduces amdxdna_cmd_get_chain() to ensure the command
count fits within the BO before returning the chain payload.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 888e857ec5582..caf7295a2b5b9 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c

[ ... ]

> @@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>  	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>  
>  	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;
> +		u32 ccnt;
> +
> +		cc = amdxdna_cmd_get_chain(abo, &ccnt);
> +		if (!cc || !ccnt)
> +			return -EINVAL;
> +
> +		cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0;
>  		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);

[Severity: High]
This isn't a bug introduced by this patch, but does this logic write the
error state to the wrong command BO?

In amdxdna_cmd_set_error(), the code correctly assigns cc->error_index based
on cmd_idx, but then it hardcodes cc->data[0] when looking up the BO. If a 
chained command fails at an index greater than 0, won't this clobber the
first command in the chain with error data and completely ignore the actual
failing command?

>  		if (!abo)
>  			return -EINVAL;
>  		cmd = amdxdna_gem_vmap(abo);
>  		if (!cmd) {

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

Re: [PATCH v2 1/2] accel/amdxdna: check the command chain payload before using it

[Lizhi Hou]

On 8/26/26 08:31, Taimuraz Kaitmazov wrote:
> amdxdna_cmd_get_payload() only bounds-checks the payload when a size
> pointer is passed, so its two callers get different guarantees from one
> function. amdxdna_cmd_set_error() takes the unchecked form and then reads
> cc->command_count and cc->data[0], neither of which has been shown to
> lie inside the BO.

AMDXDNA_CMD_EXTRA_CU_MASK is 2 bits. So cc->command_count and 
cc->data[0] will be in the BO scope.

This is IO path. I would change it only when there is a real issue.


Thanks,

Lizhi

>
> Make the size mandatory and add amdxdna_cmd_get_chain(), which returns
> the chain only once the declared command count is known to fit.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 51 ++++++++++++++++++++++-------
>   drivers/accel/amdxdna/amdxdna_ctx.h |  2 ++
>   2 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 855da8c79a1c..9f44e3918bc1 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -125,20 +125,42 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
>   	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);
> +	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);
> +
>   	return &cmd->data[num_masks];
>   }
>   
> +/*
> + * Returns the chain payload of @abo, with @count set to a command count that
> + * has been checked to fit. The chain fields live in a BO user space keeps
> + * mapped, so nothing may read them without going through here.
> + */
> +struct amdxdna_cmd_chain *
> +amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count)
> +{
> +	struct amdxdna_cmd_chain *cc;
> +	u32 len, ccnt;
> +
> +	cc = amdxdna_cmd_get_payload(abo, &len);
> +	if (!cc || len < sizeof(*cc))
> +		return NULL;
> +
> +	ccnt = READ_ONCE(cc->command_count);
> +	if (len < struct_size(cc, data, ccnt))
> +		return NULL;
> +
> +	*count = ccnt;
> +	return cc;
> +}
> +
>   u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
>   {
>   	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
> @@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>   
>   	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;
> +		u32 ccnt;
> +
> +		cc = amdxdna_cmd_get_chain(abo, &ccnt);
> +		if (!cc || !ccnt)
> +			return -EINVAL;
> +
> +		cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0;
>   		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
>   		if (!abo)
>   			return -EINVAL;
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
> index b6bef3af7dab..f6529d512217 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.h
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.h
> @@ -196,6 +196,8 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
>   }
>   
>   void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);
> +struct amdxdna_cmd_chain *
> +amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count);
>   u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo);
>   int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   			  struct amdxdna_sched_job *job, u32 cmd_idx,

[PATCH v3] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[Taimuraz Kaitmazov]

amdxdna_cmd::header packs the state, the opcode, the CU mask count and
the payload count into one u32 that lives in a BO user space keeps
mapped. The driver reads it with plain accesses, and reads it more than
once per function: amdxdna_cmd_get_payload() takes num_masks and count
from it separately, so the compiler is free to load it twice and the two
fields can come from different values.

amdxdna_cmd_set_error() and amdxdna_cmd_set_state() also read, modify and
write it in place, which lets the compiler split the store.

Read it once into a local with READ_ONCE() and write it back with
WRITE_ONCE().

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
v3:
- Dropped 1/2. You are right, EXTRA_CU_MASK is 2 bits, so the payload
  starts at most 16 bytes in and both reads land well inside a page
  granular BO.
- This patch never depended on it, so it goes standalone.

 drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++-----
 drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 888e857ec558..02027522210d 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -123,10 +123,11 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	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);
+		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
+					  READ_ONCE(cmd->header));
 
 	if (size) {
-		count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
+		count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));
 		if (unlikely(count <= num_masks ||
 			     count * sizeof(u32) +
 			     offsetof(struct amdxdna_cmd, data[0]) >
@@ -151,7 +152,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
 		return INVALID_CU_IDX;
 
-	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
+	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header));
 	cu_mask = cmd->data;
 	for (i = 0; i < num_masks; i++) {
 		if (cu_mask[i])
@@ -169,12 +170,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 	struct amdxdna_client *client = job->hwctx->client;
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
 	struct amdxdna_cmd_chain *cc = NULL;
+	u32 header;
 
 	if (!cmd)
 		return -ENOMEM;
 
-	cmd->header &= ~AMDXDNA_CMD_STATE;
-	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
+	header = READ_ONCE(cmd->header);
+	header &= ~AMDXDNA_CMD_STATE;
+	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
+	WRITE_ONCE(cmd->header, header);
 
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
 		cc = amdxdna_cmd_get_payload(abo, NULL);
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
index 6e78bab8a02c..fb440a0e2c54 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.h
+++ b/drivers/accel/amdxdna/amdxdna_ctx.h
@@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)
 	if (!cmd)
 		return ERT_INVALID_CMD;
 
-	return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header);
+	return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header));
 }
 
 static inline void
 amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum ert_cmd_state s)
 {
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
+	u32 header;
 
 	if (!cmd)
 		return;
 
-	cmd->header &= ~AMDXDNA_CMD_STATE;
-	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
+	header = READ_ONCE(cmd->header);
+	header &= ~AMDXDNA_CMD_STATE;
+	header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
+	WRITE_ONCE(cmd->header, header);
 }
 
 static inline enum ert_cmd_state
@@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
 	if (!cmd)
 		return ERT_CMD_STATE_INVALID;
 
-	return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header);
+	return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header));
 }
 
 void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);
-- 
2.55.0

Re: [PATCH v3] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[sashiko-bot]

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

New issues:
- [High] The patch fails to resolve the TOCTOU vulnerability because it issues multiple independent `READ_ONCE(cmd->header)` calls instead of storing a single local snapshot.

Pre-existing issues:
- [High] Undefined behavior caused by passing a `NULL` source pointer to `memcpy()` when parsing an empty command payload.
- [High] Uninitialized kernel stack memory is sent to the hardware mailbox, causing an information leak.
--

commit c8e0faf1355b7f70321f8abedf7d99ec0ac63c46
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

    accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

This commit attempts to fix a TOCTOU race condition in the command header
parsing by using READ_ONCE and WRITE_ONCE macros. It aims to read the
user-mapped command header once into a local variable to prevent the
compiler from splitting loads.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 888e857ec5582..02027522210d9 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -123,10 +123,11 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
>  	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);
> +		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
> +					  READ_ONCE(cmd->header));
>  
>  	if (size) {
> -		count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
> +		count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));

[Severity: High]
Does this actually fix the race condition? The commit message explicitly
claims to fix this by reading the header "once into a local with
READ_ONCE()".

Because READ_ONCE() enforces a volatile memory access, the compiler is
forced to issue multiple distinct memory loads for the opcode,
EXTRA_CU_MASK, and COUNT fields.

Could the fields still be fetched from different snapshots of the header,
maintaining the TOCTOU vulnerability?

>  		if (unlikely(count <= num_masks ||
>  			     count * sizeof(u32) +
>  			     offsetof(struct amdxdna_cmd, data[0]) >
> @@ -151,7 +152,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
>  	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
>  		return INVALID_CU_IDX;
>  
> -	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
> +	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header));

[Severity: High]
Does this suffer from the same issue? amdxdna_cmd_get_op() already reads
the header, and here it is read again with a separate READ_ONCE(), rather
than using a single local variable.

[ ... ]

> @@ -169,12 +170,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>  	struct amdxdna_client *client = job->hwctx->client;
>  	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>  	struct amdxdna_cmd_chain *cc = NULL;
> +	u32 header;
>  
>  	if (!cmd)
>  		return -ENOMEM;
>  
> -	cmd->header &= ~AMDXDNA_CMD_STATE;
> -	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	header = READ_ONCE(cmd->header);
> +	header &= ~AMDXDNA_CMD_STATE;
> +	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	WRITE_ONCE(cmd->header, header);
>  
>  	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {

[Severity: High]
Does this code read the header again after modifying it?

amdxdna_cmd_get_op() internally does a READ_ONCE(cmd->header), which
happens right after the write. Could it end up reading a newly injected
opcode if userspace modifies the header concurrently?

[Severity: High]
This isn't a bug introduced by this patch, but could this trigger undefined
behavior if a user passes an empty payload?

If a userspace application configures a command buffer with
count <= num_masks, amdxdna_cmd_get_payload() can validly return NULL:

drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_cmd_get_payload() {
    ...
		if (unlikely(count <= num_masks ||
			     count * sizeof(u32) +
			     offsetof(struct amdxdna_cmd, data[0]) >
			     abo->mem.size)) {
			*size = 0;
			return NULL;
		}
    ...
}

In drivers/accel/amdxdna/aie2_message.c:aie2_init_exec_cu_req(), the
returned pointer is passed directly to memcpy() without validation:

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

Passing a NULL pointer to memcpy() triggers undefined behavior according
to the C standard, even if the length is 0. Will this cause a kernel
panic if panic_on_warn is enabled when UBSAN catches this?

[Severity: High]
This is a pre-existing issue, but does this leak uninitialized stack
memory to the hardware mailbox?

In drivers/accel/amdxdna/aie2_message.c:aie2_execbuf(), an uninitialized
union is allocated on the stack:

	union exec_req req;
    ...
	ret = aie2_init_exec_req(&req, cmd_abo, &msg.send_size, &msg.opcode);

If aie2_init_exec_cu_req() processes a small payload (e.g. cmd_len = 0),
the remainder of the payload array retains uninitialized stack contents:

	memcpy(cu_req->payload, cmd, cmd_len);
	*size = sizeof(*cu_req);

Could the remaining 76 bytes of uninitialized kernel stack memory be
transmitted when xdna_mailbox_send_msg() is called?

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

[PATCH v2 2/2] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[Taimuraz Kaitmazov]

struct amdxdna_cmd::header packs STATE, OPCODE, COUNT and EXTRA_CU_MASK
into one u32 that lives in a BO user space keeps mapped. The driver
re-reads it on every accessor call and read-modify-writes STATE in place,
with plain accesses that let the compiler split or refetch either side.

Annotate them, as the UMQ ring indices already are. This does not make
the update atomic against user space, it only stops the compiler from
making it worse.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++-----
 drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 9f44e3918bc1..148ae51db0f1 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -123,9 +123,10 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	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);
+		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
+					  READ_ONCE(cmd->header));
 
-	count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
+	count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));
 	if (unlikely(count <= num_masks ||
 		     count * sizeof(u32) +
 		     offsetof(struct amdxdna_cmd, data[0]) >
@@ -173,7 +174,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
 		return INVALID_CU_IDX;
 
-	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
+	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header));
 	cu_mask = cmd->data;
 	for (i = 0; i < num_masks; i++) {
 		if (cu_mask[i])
@@ -191,12 +192,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 	struct amdxdna_client *client = job->hwctx->client;
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
 	struct amdxdna_cmd_chain *cc = NULL;
+	u32 header;
 
 	if (!cmd)
 		return -ENOMEM;
 
-	cmd->header &= ~AMDXDNA_CMD_STATE;
-	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
+	header = READ_ONCE(cmd->header);
+	header &= ~AMDXDNA_CMD_STATE;
+	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
+	WRITE_ONCE(cmd->header, header);
 
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
 		u32 ccnt;
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
index f6529d512217..48e1fcfd818f 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.h
+++ b/drivers/accel/amdxdna/amdxdna_ctx.h
@@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)
 	if (!cmd)
 		return ERT_INVALID_CMD;
 
-	return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header);
+	return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header));
 }
 
 static inline void
 amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum ert_cmd_state s)
 {
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
+	u32 header;
 
 	if (!cmd)
 		return;
 
-	cmd->header &= ~AMDXDNA_CMD_STATE;
-	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
+	header = READ_ONCE(cmd->header);
+	header &= ~AMDXDNA_CMD_STATE;
+	header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
+	WRITE_ONCE(cmd->header, header);
 }
 
 static inline enum ert_cmd_state
@@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
 	if (!cmd)
 		return ERT_CMD_STATE_INVALID;
 
-	return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header);
+	return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header));
 }
 
 void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);
-- 
2.55.0

Re: [PATCH v2 2/2] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[Lizhi Hou]

On 8/26/26 08:31, Taimuraz Kaitmazov wrote:
> struct amdxdna_cmd::header packs STATE, OPCODE, COUNT and EXTRA_CU_MASK
> into one u32 that lives in a BO user space keeps mapped. The driver
> re-reads it on every accessor call and read-modify-writes STATE in place,
> with plain accesses that let the compiler split or refetch either side.
>
> Annotate them, as the UMQ ring indices already are. This does not make
> the update atomic against user space, it only stops the compiler from
> making it worse.

The user space should not change the BO content after the command is 
submitted. Otherwise, the command could fail.

In another word, driver/hardware treat this as invalid command. It is ok 
as long as kernel/firmware does not crash. So the bad user application 
only messes up itself.


Thanks,

Lizhi

>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++-----
>   drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++----
>   2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 9f44e3918bc1..148ae51db0f1 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -123,9 +123,10 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
>   	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);
> +		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
> +					  READ_ONCE(cmd->header));
>   
> -	count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
> +	count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));
>   	if (unlikely(count <= num_masks ||
>   		     count * sizeof(u32) +
>   		     offsetof(struct amdxdna_cmd, data[0]) >
> @@ -173,7 +174,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo)
>   	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
>   		return INVALID_CU_IDX;
>   
> -	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
> +	num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, READ_ONCE(cmd->header));
>   	cu_mask = cmd->data;
>   	for (i = 0; i < num_masks; i++) {
>   		if (cu_mask[i])
> @@ -191,12 +192,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   	struct amdxdna_client *client = job->hwctx->client;
>   	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>   	struct amdxdna_cmd_chain *cc = NULL;
> +	u32 header;
>   
>   	if (!cmd)
>   		return -ENOMEM;
>   
> -	cmd->header &= ~AMDXDNA_CMD_STATE;
> -	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	header = READ_ONCE(cmd->header);
> +	header &= ~AMDXDNA_CMD_STATE;
> +	header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
> +	WRITE_ONCE(cmd->header, header);
>   
>   	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
>   		u32 ccnt;
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
> index f6529d512217..48e1fcfd818f 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.h
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.h
> @@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)
>   	if (!cmd)
>   		return ERT_INVALID_CMD;
>   
> -	return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header);
> +	return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header));
>   }
>   
>   static inline void
>   amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum ert_cmd_state s)
>   {
>   	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
> +	u32 header;
>   
>   	if (!cmd)
>   		return;
>   
> -	cmd->header &= ~AMDXDNA_CMD_STATE;
> -	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
> +	header = READ_ONCE(cmd->header);
> +	header &= ~AMDXDNA_CMD_STATE;
> +	header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
> +	WRITE_ONCE(cmd->header, header);
>   }
>   
>   static inline enum ert_cmd_state
> @@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
>   	if (!cmd)
>   		return ERT_CMD_STATE_INVALID;
>   
> -	return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header);
> +	return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header));
>   }
>   
>   void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size);

Re: [PATCH v2 2/2] accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header

[Taimuraz Kaitmazov]

Understood, thanks. Dropping this one, and the standalone v3 I sent 
before your reply landed.

Taimuraz

On 8/27/26 02:16, Lizhi Hou wrote:
>
> On 8/26/26 08:31, Taimuraz Kaitmazov wrote:
>> struct amdxdna_cmd::header packs STATE, OPCODE, COUNT and EXTRA_CU_MASK
>> into one u32 that lives in a BO user space keeps mapped. The driver
>> re-reads it on every accessor call and read-modify-writes STATE in 
>> place,
>> with plain accesses that let the compiler split or refetch either side.
>>
>> Annotate them, as the UMQ ring indices already are. This does not make
>> the update atomic against user space, it only stops the compiler from
>> making it worse.
>
> The user space should not change the BO content after the command is 
> submitted. Otherwise, the command could fail.
>
> In another word, driver/hardware treat this as invalid command. It is 
> ok as long as kernel/firmware does not crash. So the bad user 
> application only messes up itself.
>
>
> Thanks,
>
> Lizhi
>
>>
>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>> ---
>>   drivers/accel/amdxdna/amdxdna_ctx.c | 14 +++++++++-----
>>   drivers/accel/amdxdna/amdxdna_ctx.h | 11 +++++++----
>>   2 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c 
>> b/drivers/accel/amdxdna/amdxdna_ctx.c
>> index 9f44e3918bc1..148ae51db0f1 100644
>> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
>> @@ -123,9 +123,10 @@ void *amdxdna_cmd_get_payload(struct 
>> amdxdna_gem_obj *abo, u32 *size)
>>       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);
>> +        num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK,
>> +                      READ_ONCE(cmd->header));
>>   -    count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
>> +    count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header));
>>       if (unlikely(count <= num_masks ||
>>                count * sizeof(u32) +
>>                offsetof(struct amdxdna_cmd, data[0]) >
>> @@ -173,7 +174,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj 
>> *abo)
>>       if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN)
>>           return INVALID_CU_IDX;
>>   -    num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, 
>> cmd->header);
>> +    num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, 
>> READ_ONCE(cmd->header));
>>       cu_mask = cmd->data;
>>       for (i = 0; i < num_masks; i++) {
>>           if (cu_mask[i])
>> @@ -191,12 +192,15 @@ int amdxdna_cmd_set_error(struct 
>> amdxdna_gem_obj *abo,
>>       struct amdxdna_client *client = job->hwctx->client;
>>       struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>>       struct amdxdna_cmd_chain *cc = NULL;
>> +    u32 header;
>>         if (!cmd)
>>           return -ENOMEM;
>>   -    cmd->header &= ~AMDXDNA_CMD_STATE;
>> -    cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>> +    header = READ_ONCE(cmd->header);
>> +    header &= ~AMDXDNA_CMD_STATE;
>> +    header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>> +    WRITE_ONCE(cmd->header, header);
>>         if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
>>           u32 ccnt;
>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h 
>> b/drivers/accel/amdxdna/amdxdna_ctx.h
>> index f6529d512217..48e1fcfd818f 100644
>> --- a/drivers/accel/amdxdna/amdxdna_ctx.h
>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.h
>> @@ -169,19 +169,22 @@ amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)
>>       if (!cmd)
>>           return ERT_INVALID_CMD;
>>   -    return FIELD_GET(AMDXDNA_CMD_OPCODE, cmd->header);
>> +    return FIELD_GET(AMDXDNA_CMD_OPCODE, READ_ONCE(cmd->header));
>>   }
>>     static inline void
>>   amdxdna_cmd_set_state(struct amdxdna_gem_obj *abo, enum 
>> ert_cmd_state s)
>>   {
>>       struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>> +    u32 header;
>>         if (!cmd)
>>           return;
>>   -    cmd->header &= ~AMDXDNA_CMD_STATE;
>> -    cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
>> +    header = READ_ONCE(cmd->header);
>> +    header &= ~AMDXDNA_CMD_STATE;
>> +    header |= FIELD_PREP(AMDXDNA_CMD_STATE, s);
>> +    WRITE_ONCE(cmd->header, header);
>>   }
>>     static inline enum ert_cmd_state
>> @@ -192,7 +195,7 @@ amdxdna_cmd_get_state(struct amdxdna_gem_obj *abo)
>>       if (!cmd)
>>           return ERT_CMD_STATE_INVALID;
>>   -    return FIELD_GET(AMDXDNA_CMD_STATE, cmd->header);
>> +    return FIELD_GET(AMDXDNA_CMD_STATE, READ_ONCE(cmd->header));
>>   }
>>     void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 
>> *size);