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

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,

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