Re: [PATCH V2 14/20] accel/amdxdna: Implement AIE4 command packet building and submission

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] accel/amdxdna: TOCTOU out-of-bounds read/write in amdxdna_cmd_get_payload()
- [High] accel/amdxdna: chained sub-command BOs bypass fence attachment
--

commit c176cc9653a37bd631df8131c299b9255f903027
Author: David Zhang <yidong.zhang@amd.com>

accel/amdxdna: Implement AIE4 command packet building and submission

This commit implements kernel-mode command submission and hardware queue
packet assembly for AIE4. It introduces aie4_cmd_submit() to validate
incoming command buffers, lock reservations, and dispatch to the hardware
queue while tracking job completions.

> diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c
> --- a/drivers/accel/amdxdna/aie4_ctx.c
> +++ b/drivers/accel/amdxdna/aie4_ctx.c
[ ... ]
> @@ -731,21 +731,21 @@ static int submit_one_cmd(struct amdxdna_hwctx *hwctx,
>  			  struct amdxdna_gem_obj *cmd_abo, bool last_of_chain,
>  			  u64 *seq)
>  {
>  	struct amdxdna_hwctx_priv *priv = hwctx->priv;
>  	struct amdxdna_dev *xdna = hwctx->client->xdna;
>  	struct amdxdna_cmd_start_dpu *dpu;
>  	struct host_queue_packet *pkt;
>  	u64 slot_idx;
>  	u16 chained;
>  	int ret;
>  
>  	ret = validate_cmd_abo(xdna, cmd_abo, &chained);
>  	if (ret)
>  		return ret;
>  
>  	dpu = amdxdna_cmd_get_payload(cmd_abo, NULL);

[Severity: Critical]
Is there a TOCTOU race window here between validation and payload retrieval?

Userspace can concurrently modify cmd_abo (which is a mapped BO) between
the bounds checks in validate_cmd_abo() and this call to
amdxdna_cmd_get_payload().

When the size argument is passed as NULL, amdxdna_cmd_get_payload() skips
its internal size bounds checks and computes the pointer offset directly
from the potentially modified header (AMDXDNA_CMD_EXTRA_CU_MASK).

This could allow it to return a pointer significantly out of bounds,
leading to an out-of-bounds read when dpu is subsequently accessed, or
potentially leaking arbitrary kernel memory to the hardware queue.

[ ... ]
> @@ -911,21 +911,21 @@ static int submit_job_cmds(struct amdxdna_hwctx *hwctx,
>  			   struct amdxdna_sched_job *job, u32 op)
>  {
[ ... ]
>  	/* Validate all sub-command BOs and payloads before dispatching packets. */
>  	for (i = 0; i < ccnt; i++) {
>  		u32 boh = (u32)(payload->data[i]);
>  
>  		job->aie4_cmd_bos[i] = amdxdna_gem_get_obj(hwctx->client, boh, AMDXDNA_BO_SHARE);
>  		if (!job->aie4_cmd_bos[i]) {
>  			XDNA_ERR(xdna, "Failed to find cmd BO %u at index %u", boh, i);
>  			ret = -ENOENT;
>  			goto err_put_bos;
>  		}

[Severity: High]
Does this skip dma_resv fence attachment and mapping checks for sub-command
BOs?

Userspace can submit an ERT_CMD_CHAIN job with sub-command handles in the
payload that are omitted from the ioctl's top-level args->bos array.

Because these sub-command BOs are fetched dynamically here from user handles,
their reservations aren't locked and map_invalid is never checked before
dispatch.

[ ... ]
> @@ -1050,15 +1050,15 @@ int aie4_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job,
[ ... ]
>  	job->out_fence = dma_fence_get(job->fence);
>  	for (i = 0; i < job->bo_cnt; i++)
>  		dma_resv_add_fence(job->bos[i]->resv, job->out_fence, DMA_RESV_USAGE_WRITE);

[Severity: High]
Are the dynamically fetched sub-command BOs missing their execution fences?

The execution fences are exclusively attached to BOs present in the top-level
job->bos array.

Since the dynamically fetched sub-command BOs in job->aie4_cmd_bos bypass this
fence attachment, hardware DMA can occur on BOs without a corresponding
dma_resv execution fence.

This might lead the memory manager to consider these BOs idle, potentially
reclaiming, migrating, or unmapping their pages while the device is actively
writing to them.

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