Re: [PATCH] accel/amdxdna: reject a command chain that carries no commands

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

Pre-existing issues:
- [Critical] Local Denial of Service (DoS) via zero-size buffer in AMDXDNA_SYNC_BO ioctl.
- [Critical] Missing NULL pointer check after `amdxdna_cmd_get_payload()` leads to uninitialized variable use and NULL pointer dereference.
--

--- Patch [1]: [PATCH] accel/amdxdna: reject a command chain that carries no commands ---
commit 28fe797436c130cd969581d26479f7eb0ffdc5a9
Author: Taimuraz Kaitmazov <[email protected]>

accel/amdxdna: reject a command chain that carries no commands

A chain whose command_count is zero passes the payload length check,
because struct_size(payload, data, 0) is just the header. The fill loop
then does not run, so offset stays zero and the request is submitted with
a zero-length buffer.

[ ... ]

> drm_clflush_virt_range() faulting on an empty range is a core problem, and a
> patch for it is on the list separately. This rejects the request in the driver
> regardless, since a chain carrying no commands is not something to submit.

[Severity: Critical]
This isn't a bug introduced by this patch, but since
drm_clflush_virt_range() faulting on a zero-size range is known to cause a
kernel panic, is there another path that might trigger this before the core
fix lands?

Userspace can create a non-DEV BO (e.g., AMDXDNA_BO_SHMEM) and call the
AMDXDNA_SYNC_BO ioctl with size = 0. The ioctl handler lacks a zero-size
check and passes the unmodified user input:

amdxdna_drm_sync_bo_ioctl() {
    ...
    ret = amdxdna_flush_bo(abo, args->offset, args->size);
    ...
}

amdxdna_flush_bo() {
    ...
    drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
}

Could this allow an unprivileged user to bypass the driver fix here and
trigger the vmap guard page fault anyway?

> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
> index dfe0fbdf066d2..b4c49259a1a23 100644
> --- a/drivers/accel/amdxdna/aie2_message.c
> +++ b/drivers/accel/amdxdna/aie2_message.c
> @@ -994,7 +994,7 @@ int aie2_cmdlist_multi_execbuf(struct amdxdna_hwctx *hwctx,
>  	}
>  
>  	ccnt = payload->command_count;
> -	if (payload_len < struct_size(payload, data, ccnt)) {
> +	if (!ccnt || payload_len < struct_size(payload, data, ccnt)) {

[Severity: Critical]
This isn't a bug introduced by this patch, but while looking at payload
validation here, are there missing NULL checks for
amdxdna_cmd_get_payload() in other parsers?

In aie2_cmdlist_multi_execbuf() above this code, there is a check:

aie2_cmdlist_multi_execbuf() {
    ...
    payload = amdxdna_cmd_get_payload(cmd_abo, &payload_len);
    if (!payload) {
        ...
}

But other payload parsing functions like aie2_cmdlist_fill_npu_cf() and
aie2_init_exec_cu_req() appear to lack this check:

aie2_cmdlist_fill_npu_cf() {
    ...
    cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
    if (*size < sizeof(*npu_slot) + cmd_len)
        return -EINVAL;
    ...
    memcpy(npu_slot->args, cmd, cmd_len);
}

If amdxdna_gem_vmap() fails under memory pressure and returns NULL,
amdxdna_cmd_get_payload() will return NULL and leave cmd_len uninitialized.
Could this lead to an uninitialized size check and subsequent NULL pointer
dereference in memcpy()?

>  		XDNA_DBG(xdna, "Invalid command count %d", ccnt);
>  		return -EINVAL;
>  	}

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