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

[sashiko-bot@kernel.org]

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