Re: [PATCH V1] accel/amdxdna: Remove __counted_by from struct amdxdna_cmd_chain

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

Pre-existing issues:
- [High] Reference count leak of a GEM object on an error path in `amdxdna_cmd_set_error()`.
- [High] Calling `memcpy` with a NULL source pointer when processing empty payloads.
- [High] Missing READ_ONCE() on shared memory reads allows compiler reloads, causing a Time-of-Check to Time-of-Use (TOCTOU) vulnerability that bypasses bounds checks.
--

--- Patch [1]: [PATCH V1] accel/amdxdna: Remove __counted_by from struct amdxdna_cmd_chain ---
commit fcdbc0a8d70945b3b3d7614f4b69f29895cd2af9
Author: Lizhi Hou <[email protected]>

accel/amdxdna: Remove __counted_by from struct amdxdna_cmd_chain

The commit removes the __counted_by annotation from the data array in
struct amdxdna_cmd_chain. This prevents kernel panics caused by bounds
checks failing if userspace concurrently modifies command_count to zero
in shared memory.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
> index b6bef3af7dab4..6e78bab8a02c0 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.h
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.h
> @@ -55,7 +55,7 @@ struct amdxdna_cmd_chain {
>  	u32 submit_index;
>  	u32 error_index;
>  	u32 reserved[3];
> -	u64 data[] __counted_by(command_count);
> +	u64 data[];
>  };

[Severity: High]
This is a pre-existing issue, but since this patch addresses concurrent
userspace modifications to command_count, are there other Time-of-Check to
Time-of-Use (TOCTOU) issues with this field?

Looking at drivers/accel/amdxdna/aie2_message.c:aie2_cmdlist_multi_execbuf():

	ccnt = payload->command_count;
	if (payload_len < struct_size(payload, data, ccnt)) {
		XDNA_DBG(xdna, "Invalid command count %d", ccnt);
		return -EINVAL;
	}

	op = ERT_INVALID_CMD;
	for (i = 0; i < ccnt; i++) {
		u32 boh = (u32)(payload->data[i]);

Can the compiler reload payload->command_count inside the loop condition
since READ_ONCE() is not used? If userspace changes the value to a massive
number after the payload_len check passes, could this bypass the bounds
check and cause out-of-bounds reads on adjacent kernel memory?

A similar issue appears in
drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_cmd_get_payload() where
cmd->header is read multiple times:

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

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

Could a compiler reload cause the calculated size to exceed the BO size?


[Severity: High]
This is a pre-existing issue, but does amdxdna_cmd_get_payload() return NULL
if the payload is empty?

In drivers/accel/amdxdna/aie2_message.c:aie2_init_exec_cu_req():

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

If userspace constructs a command BO where count <= num_masks,
amdxdna_cmd_get_payload() sets cmd_len to 0 and returns NULL. If a valid CU
index is still returned, this calls memcpy(cu_req->payload, NULL, 0).

Could this undefined behavior reliably crash the kernel if the UBSAN bounds
checker with panic_on_warn is enabled?

The same pattern seems to exist in aie2_cmdlist_fill_cf() and
aie2_cmdlist_fill_npu_cf().


[Severity: High]
This is a pre-existing issue, but is there a reference leak on the error path
in amdxdna_cmd_set_error()?

In drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_cmd_set_error():

	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;
		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(abo) fails, the function returns -ENOMEM early. Should
there be an amdxdna_gem_put_obj(abo) call to avoid leaking the GEM object
reference acquired by amdxdna_gem_get_obj()? Could a malicious userspace
repeatedly supply a BO handle that fails to map to cause unfreeable memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1