Re: [PATCH v3] 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:

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