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@xxxxxxxxxxxxx>
> ---
>   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,