[PATCH] accel/amdxdna: check the command payload before using it

[PATCH] accel/amdxdna: check the command payload before using it

[Taimuraz Kaitmazov]

amdxdna_cmd_get_payload() answers NULL when the command BO cannot be
mapped, and on that path it returns before writing *size, so the caller's
length keeps whatever the stack held. Its eight callers in aie2_message.c
use both without checking either:

	cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
	if (cmd_len > sizeof(cu_req->payload))
		return -EINVAL;
	...
	memcpy(cu_req->payload, cmd, cmd_len);

so an uninitialised length decides the bounds check and then a NULL
source is copied from. The mapping fails under vmalloc pressure today,
and any future reason for amdxdna_gem_vmap() to refuse widens it.

Check the pointer, and clear the length before the early return so a
caller that ignores the pointer still reads a defined value.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/aie2_message.c | 16 ++++++++++++++++
 drivers/accel/amdxdna/amdxdna_ctx.c  |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
index dfe0fbdf066d..6f258206d508 100644
--- a/drivers/accel/amdxdna/aie2_message.c
+++ b/drivers/accel/amdxdna/aie2_message.c
@@ -555,6 +555,8 @@ static int aie2_init_exec_cu_req(struct amdxdna_gem_obj *cmd_bo, void *req,
 	void *cmd;
 
 	cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!cmd)
+		return -EINVAL;
 	if (cmd_len > sizeof(cu_req->payload))
 		return -EINVAL;
 
@@ -577,6 +579,8 @@ static int aie2_init_exec_dpu_req(struct amdxdna_gem_obj *cmd_bo, void *req,
 	u32 cmd_len;
 
 	sn = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!sn)
+		return -EINVAL;
 	if (cmd_len - sizeof(*sn) > sizeof(dpu_req->payload))
 		return -EINVAL;
 
@@ -622,6 +626,8 @@ aie2_cmdlist_fill_cf(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t *size)
 	void *cmd;
 
 	cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!cmd)
+		return -EINVAL;
 	if (*size < sizeof(*cf_slot) + cmd_len)
 		return -EINVAL;
 
@@ -645,6 +651,8 @@ aie2_cmdlist_fill_dpu(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t *size)
 	u32 arg_sz;
 
 	sn = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!sn)
+		return -EINVAL;
 	arg_sz = cmd_len - sizeof(*sn);
 	if (cmd_len < sizeof(*sn) || arg_sz > MAX_DPU_ARGS_SIZE)
 		return -EINVAL;
@@ -705,6 +713,8 @@ aie2_cmdlist_fill_npu_cf(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t *siz
 	void *cmd;
 
 	cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!cmd)
+		return -EINVAL;
 	if (*size < sizeof(*npu_slot) + cmd_len)
 		return -EINVAL;
 
@@ -730,6 +740,8 @@ aie2_cmdlist_fill_npu_dpu(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t *si
 	u32 arg_sz;
 
 	sn = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!sn)
+		return -EINVAL;
 	arg_sz = cmd_len - sizeof(*sn);
 	if (cmd_len < sizeof(*sn) || arg_sz > MAX_NPU_ARGS_SIZE)
 		return -EINVAL;
@@ -762,6 +774,8 @@ aie2_cmdlist_fill_npu_preempt(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t
 	u32 arg_sz;
 
 	pd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!pd)
+		return -EINVAL;
 	arg_sz = cmd_len - sizeof(*pd);
 	if (cmd_len < sizeof(*pd) || arg_sz > MAX_NPU_ARGS_SIZE)
 		return -EINVAL;
@@ -798,6 +812,8 @@ aie2_cmdlist_fill_npu_elf(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t *si
 	u32 arg_sz;
 
 	pd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
+	if (!pd)
+		return -EINVAL;
 	arg_sz = cmd_len - sizeof(*pd);
 	if (cmd_len < sizeof(*pd) || arg_sz > MAX_NPU_ARGS_SIZE)
 		return -EINVAL;
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 31a414c3f0d9..505c249e142e 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -117,6 +117,9 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
 	u32 num_masks, count;
 
+	if (size)
+		*size = 0;
+
 	if (!cmd)
 		return NULL;
 
-- 
2.55.0

Re: [PATCH] accel/amdxdna: check the command payload before using it

[sashiko-bot]

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

Pre-existing issues:
- [Critical] An out-of-bounds write of 4 bytes in `aie2_cmdlist_fill_npu_elf` leads to an integer underflow in `aie2_cmdlist_multi_execbuf`, enabling massive out-of-bounds memory corruption.
- [High] Leaked GEM object reference on error path in amdxdna_cmd_set_error().
--

--- Patch [1]: [PATCH] accel/amdxdna: check the command payload before using it ---
commit ec2f075309e2dcb4167331b4d2cc9a7b02abaa4a
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: check the command payload before using it

This commit fixes an issue where amdxdna_cmd_get_payload() could return NULL
without setting the size output parameter. Callers in aie2_message.c were using
the uninitialized size for bounds checking and copying from the NULL pointer.
The fix clears the length before an early return and adds checks for the NULL
pointer in callers.

> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
> index dfe0fbdf066d2..6f258206d5080 100644
> --- a/drivers/accel/amdxdna/aie2_message.c
> +++ b/drivers/accel/amdxdna/aie2_message.c

[ ... ]

> @@ -798,6 +812,8 @@ aie2_cmdlist_fill_npu_elf(struct amdxdna_gem_obj *cmd_bo, void *slot, size_t *si
>  	u32 arg_sz;
>  
>  	pd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
> +	if (!pd)
> +		return -EINVAL;
>  	arg_sz = cmd_len - sizeof(*pd);
>  	if (cmd_len < sizeof(*pd) || arg_sz > MAX_NPU_ARGS_SIZE)
>  		return -EINVAL;

[Severity: Critical]
This is a pre-existing issue, but if arg_sz is 0, the bounds check incorrectly
permits a size of exactly sizeof(*npu_slot), yet args[0] is unconditionally
written:

drivers/accel/amdxdna/aie2_message.c:aie2_cmdlist_fill_npu_elf() {
    ...
	if (*size < sizeof(*npu_slot) + arg_sz)
		return -EINVAL;

	memset(npu_slot, 0, sizeof(*npu_slot));
    ...
	npu_slot->arg_cnt = 1;
	npu_slot->args[0] = AIE2_EXEC_BUFFER_KERNEL_OP_TXN;

	*size = struct_size(npu_slot, args, npu_slot->arg_cnt);
    ...
}

Does this unconditionally write args[0], causing a 4-byte out-of-bounds
write?

And if so, does it also lead to an integer underflow in
aie2_cmdlist_multi_execbuf()?

drivers/accel/amdxdna/aie2_message.c:aie2_cmdlist_multi_execbuf() {
    ...
		size = cmdbuf_abo->mem.size - offset;
		ret = aie2_cmdlist_fill_slot(cmd_buf + offset, abo, &size, &op);
		amdxdna_gem_put_obj(abo);
		if (ret)
			return ret;

		offset += size;
    ...
}

If the previous 4-byte overflow artificially inflates size, can offset exceed
cmdbuf_abo->mem.size, causing size to underflow on the next loop iteration
and disable bounds checks for subsequent payloads?

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 31a414c3f0d96..505c249e142e6 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -117,6 +117,9 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
>  	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
>  	u32 num_masks, count;
>  
> +	if (size)
> +		*size = 0;
> +
>  	if (!cmd)
>  		return NULL;
>  

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

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() fails, it returns -ENOMEM without calling
amdxdna_gem_put_obj() to release the abo obtained from amdxdna_gem_get_obj().
Does this lead to a memory leak of the GEM object under memory pressure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818002459.377641-1-taimuraz@kaitmazov.com?part=1