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

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 <[email protected]>

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/[email protected]?part=1