Re: [PATCH V3 16/19] accel/amdxdna: Implement AIE4 command packet building and submission

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] amdxdna: DMA UAF via user-writable UMQ read index spoofing
- [High] amdxdna: out-of-bounds read of dpu->chained before payload size validation
- [High] drm: dma_fence_context_alloc() per job breaks dma_resv timeline coalescing
--

commit 2abb65f20d471579d835973557557cc2b94cacf8
Author: David Zhang <yidong.zhang@amd.com>
Subject: accel/amdxdna: Implement AIE4 command packet building and submission

This patch implements kernel-mode command submission and hardware queue packet
assembly for AIE4. It adds functions to validate command buffers, build
direct and indirect packets, and manage fence contexts for jobs. It also
holds references to sub-command BOs during chained submissions to prevent
use-after-free conditions.

> diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c
> index 09c92b1b5134e..31bf30b9dc86a 100644
> --- a/drivers/accel/amdxdna/aie4_ctx.c
> +++ b/drivers/accel/amdxdna/aie4_ctx.c

[ ... ]

> @@ -469,3 +502,717 @@ int aie4_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq, u32 timeout)

[ ... ]

> +static int validate_cmd_abo(struct amdxdna_dev *xdna, struct amdxdna_gem_obj *cmd_abo,
> +			    struct amdxdna_cmd_start_dpu **dpu_out, u16 *chained_cnt)
> +{
> +	struct amdxdna_cmd_start_dpu *dpu;
> +	u32 payload_size;
> +	u16 chained;
> +	u32 op;
> +	u16 i;
> +
> +	op = amdxdna_cmd_get_op(cmd_abo);
> +	if (op != ERT_START_DPU) {
> +		XDNA_ERR(xdna, "Invalid exec buf op, %d", op);
> +		return -EINVAL;
> +	}
> +
> +	dpu = amdxdna_cmd_get_payload(cmd_abo, &payload_size);
> +	if (!dpu) {
> +		XDNA_ERR(xdna, "Invalid DPU payload");
> +		return -EINVAL;
> +	}
> +	chained = READ_ONCE(dpu->chained);
> +	if (chained >= HSA_MAX_LEVEL1_INDIRECT_ENTRIES ||
> +	    payload_size < (u32)(chained + 1) * sizeof(*dpu)) {

[Severity: High]
Does this code risk an out-of-bounds read if the user supplies a command BO
where the payload pointer falls at the very end of a mapped page?

The speculative read of dpu->chained happens before the payload_size is
validated to be at least sizeof(*dpu). If the payload leaves fewer bytes
than sizeof(*dpu) at the end of a page and the adjacent page is unmapped,
accessing dpu->chained could cause a fatal kernel page fault.

Should the payload_size check for at least sizeof(*dpu) happen before
accessing any fields from dpu?

> +		XDNA_ERR(xdna, "Invalid DPU chained entries %u, payload %u",
> +			 chained, payload_size);
> +		return -EINVAL;
> +	}

[ ... ]

> +static void job_worker(struct work_struct *work)
> +{
> +	struct amdxdna_hwctx_priv *priv =
> +		container_of(work, struct amdxdna_hwctx_priv, job_work);
> +	struct amdxdna_hwctx *hwctx = priv->hwctx;
> +	struct amdxdna_sched_job *job;
> +
> +	while ((job = peek_running_job(hwctx))) {
> +		wait_till_seq_completed(hwctx, job->seq);
> +		if (get_read_index(hwctx) > job->seq) {

[Severity: Critical]
Can this implementation be tricked by userspace into releasing BOs while the
hardware is still executing?

The kernel determines job completion by reading the hardware completion status
from a memory location via get_read_index(), which points to a GEM object
allocated by the user.

If malicious userspace maps this object as writable and manually advances the
read_index to match the write_index, the worker would see completion and
call job_complete(). This would prematurely signal the job's fence and drop
references to the command and argument BOs.

Since the fence is signaled, userspace can free the BOs immediately, causing
the physical pages to be returned to the system while the NPU hardware might
continue executing and DMA-ing to those pages. Could this lead to memory
corruption on systems using iommu=pt?

> +			dequeue_running_job(hwctx, job);
> +			/* Abort partially submitted jobs; complete fully submitted ones. */
> +			if (job->aie4_job_state != AIE4_JOB_STATE_SUBMITTED)
> +				job_abort(job);
> +			else
> +				job_complete(job);
> +		} else if (aie4_hwctx_has_error(hwctx)) {
> +			dequeue_running_job(hwctx, job);
> +			job_abort(job);
> +		} else {
> +			/* suspend/resume */
> +			break;
> +		}
> +	}
> +}

[ ... ]

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 888e857ec5582..608b86fe8f6bb 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c

[ ... ]

> @@ -55,9 +51,9 @@ static struct dma_fence *amdxdna_fence_create(struct amdxdna_hwctx *hwctx)
>  	if (!fence)
>  		return NULL;
>  
> -	fence->hwctx = hwctx;
>  	spin_lock_init(&fence->lock);
> -	dma_fence_init(&fence->base, &fence_ops, &fence->lock, hwctx->id, 0);
> +	/* Unique timeline context prevents eviction from shared BO reservation. */
> +	dma_fence_init(&fence->base, &fence_ops, &fence->lock, dma_fence_context_alloc(1), 0);

[Severity: High]
Can allocating a unique context per job break dma_resv timeline coalescing
and cause unbounded kernel memory allocation?

By using dma_fence_context_alloc(1) for every job, each job gets a unique
timeline context. When dma_resv_add_fence() adds these fences to a shared BO
in fence_job_bos(), it cannot identify older fences as belonging to the same
timeline because dma_fence_is_later_or_same() will fail to match contexts.

If userspace spawns multiple threads to concurrently submit jobs referencing
the same shared BO, could dma_resv accumulate every unsignaled fence and grow
its internal array linearly until the jobs complete, potentially leading to
OOM or page allocation failures?

>  	return &fence->base;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008032348.2044667-1-yidong.zhang@amd.com?part=16