Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] accel/amdxdna: TOCTOU in submit_job_cmds allows arbitrary DMA to freed memory (UAF) - [High] accel/amdxdna: Unhandled mid-chain submission failure permanently hangs dma_fence - [High] accel/amdxdna: dtrace_buffer address truncated to 48 bits, corrupting memory - [High] accel/amdxdna: dma_fence uses unreferenced struct device pointer causing UAF - [High] accel/amdxdna: Submitter holding SRCU lock waits forever for reconnect, deadlocking teardown - [High] accel/amdxdna: Missing READ_ONCE allows double-fetch of user-shared control fields --
commit 3eb430c5785965c7805a7d4c133aa9e24f11869b Author: David Zhang <[email protected]> accel/amdxdna: Implement AIE4 command packet building and submission Implement kernel-mode command submission and hardware queue packet assembly for AIE4. > 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 [ ... ] > @@ -622,7 +622,10 @@ static int wait_till_connected_hsa_not_full(struct > amdxdna_hwctx *hwctx, > if (!ret) > hsa_not_full = true; > } > ret = wait_event_freezable(priv->job_list_wq, > aie4_hwctx_connected(hwctx) || > (!wait_through_reset && > aie4_hwctx_has_reset(hwctx))); [Severity: High] Can this block indefinitely and cause an SRCU deadlock? When wait_through_reset is true for the first command, the wait condition completely ignores the reset or error state. Since the submitter thread holds the hwctx_srcu read lock, if a device unplug occurs and the teardown thread calls synchronize_srcu(), the teardown will wait forever for this lock to be released, while this code waits for a reconnect that will never happen. > mutex_lock(&priv->io_lock); [ ... ] > @@ -696,7 +699,8 @@ static int fill_indirect_pkt(struct amdxdna_hwctx_priv > *priv, u64 slot_idx, > for (i = 0; i < entries; i++, dpu++, hipe++) { > struct host_indirect_packet_data *hipd; > u64 indirect_pkt_dev_addr; > u32 uci = dpu->uc_index; [Severity: High] Can the compiler double-fetch dpu->uc_index from shared memory here? Because READ_ONCE() is missing, a concurrent userspace thread could modify the value immediately after the bounds check against HSA_MAX_LEVEL1_INDIRECT_ENTRIES, allowing an out-of-bounds array index into priv->umq_indirect_pkts[idx] later in this function. > u32 idx; > > /* [ ... ] > @@ -715,7 +718,8 @@ static int fill_indirect_pkt(struct amdxdna_hwctx_priv > *priv, u64 slot_idx, > upper_32_bits(dpu->instruction_buffer); > hipd->payload.dtrace_buf_host_addr_low = > lower_32_bits(dpu->dtrace_buffer); > hipd->payload.dtrace_buf_host_addr_high = > lower_16_bits(upper_32_bits(dpu->dtrace_buffer)); [Severity: High] Does this truncate the 64-bit user address to 48 bits? By using lower_16_bits(upper_32_bits()), bits 48-63 of the virtual address are silently dropped. On SVA/IOMMU platforms with 5-level paging, user virtual addresses can reach 57 bits, which might cause the hardware to write trace data to unintended memory locations. > } > pkt->pkt_header.common_header.distribute = 1; [ ... ] > @@ -730,7 +732,8 @@ static void fill_direct_pkt(struct amdxdna_hwctx_priv > *priv, u64 slot_idx, > ebuf->dpu_control_code_host_addr_high = > upper_32_bits(dpu->instruction_buffer); > ebuf->dtrace_buf_host_addr_low = lower_32_bits(dpu->dtrace_buffer); > ebuf->dtrace_buf_host_addr_high = > lower_16_bits(upper_32_bits(dpu->dtrace_buffer)); [Severity: High] Similar to fill_indirect_pkt(), can this truncate the user address to 48 bits, potentially corrupting memory on systems with 5-level paging? > pkt->pkt_header.common_header.distribute = 0; [ ... ] > @@ -772,7 +774,8 @@ static int submit_one_cmd(struct amdxdna_hwctx *hwctx, > * against the actual payload size before dereferencing chained+1 DPU > * entries, so a bogus count cannot drive an out-of-bounds read. > */ > chained = dpu->chained; [Severity: High] Could the lack of READ_ONCE() here allow a double-fetch of dpu->chained? If userspace modifies this value after the bounds check, it could bypass the validation. > if (chained >= HSA_MAX_LEVEL1_INDIRECT_ENTRIES) { > XDNA_ERR(xdna, "Invalid DPU data"); [ ... ] > @@ -1012,7 +1014,8 @@ static int submit_job_cmds(struct amdxdna_hwctx *hwctx, > XDNA_ERR(xdna, "Invalid cmd payload for chained cmd"); > return -EINVAL; > } > ccnt = payload->command_count; [Severity: High] Since payload points to a GEM object mapped into user memory, can the compiler optimize this and re-fetch payload->command_count after the bounds validation? Without READ_ONCE(), userspace could potentially modify the count concurrently to bypass the checks and trigger out-of-bounds reads. > /* > * A chain (runlist) must fit within the queue. CERT advances the > host-visible [ ... ] > @@ -1032,15 +1034,22 @@ static int submit_job_cmds(struct amdxdna_hwctx > *hwctx, > for (i = 0; i < ccnt; i++) { > u32 boh = (u32)(payload->data[i]); > struct amdxdna_gem_obj *abo; > > abo = amdxdna_gem_get_obj(hwctx->client, boh, AMDXDNA_BO_SHARE); > if (!abo) { > XDNA_ERR(xdna, "Failed to find cmd BO %u", boh); > ret = -ENOENT; > break; > } > > /* > * submit_one_cmd() blocks in > wait_till_connected_hsa_not_full() until the > * ctx is connected and a slot is free, so a concurrent > suspend/disconnect > * is waited out inline rather than returned here. The first > sub-command > * (i == 0, nothing published yet) waits through a TDR reset > and runs on > * the recreated ctx; a later sub-command returns -ECONNRESET > if a reset > * landed while waiting for a slot, so the published prefix is > not split > * across the reset. It also returns -ERESTARTSYS on a signal, > or a > * validation error. Break on any; a published prefix is then > reaped by > * the job worker's reset drain (see below). > */ > ret = submit_one_cmd(hwctx, abo, i + 1 == ccnt, i == 0, > &job->seq); > amdxdna_gem_put_obj(abo); [Severity: Critical] Does this allow arbitrary DMA to freed memory? The driver reads the BO handle (boh) directly from the user-shared GEM object. Userspace can modify this handle concurrently after the DRM scheduler validates job->bos. Because this substituted BO is not part of job->bos, it lacks a tracking fence. By immediately dropping the kernel reference via amdxdna_gem_put_obj(), userspace can close the handle and free the memory while the hardware asynchronously executes DMA operations on the freed physical memory. > if (ret) > break; [ ... ] > @@ -1054,6 +1056,7 @@ static int submit_job_cmds(struct amdxdna_hwctx *hwctx, > * published prefix and reports the partial chain as failed (ABORT). > Only when > * nothing was published (i == 0) is the error returned to the caller. > */ > if (i > 0) > return 0; [Severity: High] If submit_one_cmd() fails for a sub-command mid-chain (e.g., due to an invalid payload) after the first command is published, this masks the failure and returns success. Since the final command is never published, CHAIN_FLG_LAST_CMD is never sent to the hardware. Will this cause the hardware to wait indefinitely, preventing job_done() from ever signaling the dma_fence? > > return ret; > } [ ... ] > @@ -1182,7 +1184,8 @@ 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] Following up on the mid-chain failure issue in submit_job_cmds(), since the fence is attached to shared BOs here, if it never signals due to an abandoned hardware queue, won't any future access to those BOs by other processes hang forever, creating a denial of service? > > up_read(&xdna->notifier_lock); > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c > b/drivers/accel/amdxdna/amdxdna_ctx.c > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c [ ... ] > @@ -47,7 +49,8 @@ static const char *amdxdna_fence_get_timeline_name(struct > dma_fence *fence) > * another process). The device outlives any individual context, so > * dev_name() is safe to call at any point during the fence's lifetime. > */ > return dev_name(xdna_fence->dev); [Severity: High] Can this trigger a use-after-free if the sync_file outlives the DRM device? Userspace querying the timeline after the device has been torn down or hot-unplugged would access the freed struct device pointer here. > } > [ ... ] > @@ -62,7 +65,8 @@ static struct dma_fence *amdxdna_fence_create(struct > amdxdna_hwctx *hwctx) > if (!fence) > return NULL; > > fence->dev = hwctx->client->xdna->ddev.dev; [Severity: High] Does this store the struct device pointer without taking a reference via get_device()? This allows the use-after-free mentioned above when the fence outlives the device. > spin_lock_init(&fence->lock); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
