Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Returning `-EINTR` while leaving the software state modified breaks standard ioctl restart semantics. Pre-existing issues: - [High] Hardware Page Fault / Use-After-Free when debug BO is closed while attached. - [High] Missing Error Handling in aie2_cmd_wait allows state desynchronization. - [High] Denial of Service via device-wide client_lock during unbounded wait. -- commit 5562369312b08033baebb62691f056cab3d5a6c1 Author: Taimuraz Kaitmazov <[email protected]> Subject: accel/amdxdna: make the debug BO command wait interruptible This commit changes aie2_cmd_wait() to wait interruptibly instead of uninterruptibly, returning -EINTR on interruption. It also leaves the BO attached when interrupted to prevent detachment failures if the hardware is successfully attached. > diff --git a/drivers/accel/amdxdna/aie2_ctx.c > b/drivers/accel/amdxdna/aie2_ctx.c > index baf9a8b90a4d6..d3c80f760a1d3 100644 > --- a/drivers/accel/amdxdna/aie2_ctx.c > +++ b/drivers/accel/amdxdna/aie2_ctx.c > @@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx > *hwctx, void *buf, u32 size > return ret; > } > > -static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq) > +static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq) > { > struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq); > struct amdxdna_dev *xdna = hwctx->client->xdna; > + long ret; > > if (!out_fence) { > XDNA_ERR(xdna, "Failed to get fence"); > - return; > + return 0; [Severity: High] This is a pre-existing issue, but does returning 0 here indicate success to the caller despite the inability to wait for the command? If aie2_cmd_get_out_fence() fails (e.g., an unprivileged user concurrently resets hwctx->syncobj_hdl), the driver skips waiting for the hardware but proceeds as if the command finished. It seems this would cause subsequent logic to evaluate an uninitialized or zeroed cmd->result, leading to software state desynchronization. > } > > mutex_unlock(&xdna->dev_lock); > - dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT); > + ret = dma_fence_wait_timeout(out_fence, true, MAX_SCHEDULE_TIMEOUT); [Severity: High] This is a pre-existing issue, but is it safe to sleep here for potentially an unbounded amount of time without dropping the device-wide client_lock? While dev_lock is explicitly dropped, the caller in amdxdna_drm_config_hwctx_ioctl() acquires the device-wide client_lock before initiating the wait: drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_drm_config_hwctx_ioctl() { ... mutex_lock(&xdna->client_lock); mutex_lock(&xdna->dev_lock); ... } An unprivileged user could submit a long-running compute job, issue a config ioctl (e.g., ATTACH) that waits behind the compute job, and simply let their thread sleep without sending a signal. This would keep the device-wide client_lock held indefinitely, blocking all other users from opening or closing the device. > mutex_lock(&xdna->dev_lock); > dma_fence_put(out_fence); [ ... ] > @@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx > *hwctx, u32 bo_hdl, > goto put_cmd; > } > > - aie2_cmd_wait(hwctx, seq); > + ret = aie2_cmd_wait(hwctx, seq); > + if (ret) { > + /* > + * The command may still attach the BO. Record it as attached, > + * or DETACH will refuse it and there is no way back. > + */ > + if (attach) > + abo->assigned_hwctx = hwctx->id; > + goto put_cmd; > + } > + > if (cmd->result) { > XDNA_ERR(xdna, "Response failure 0x%x", cmd->result); > ret = -EINVAL; [Severity: High] Does returning -EINTR while leaving the software state modified break standard ioctl restart semantics? When aie2_cmd_wait() is interrupted, userspace receives -EINTR and will typically retry the identical syscall. If an interrupted ATTACH_DEBUG_BO proactively sets abo->assigned_hwctx, userspace treating -EINTR as a failure will not issue a subsequent DETACH, permanently stranding the BO. If a DETACH_DEBUG_BO is interrupted, abo->assigned_hwctx intentionally skips cleanup. If the hardware successfully detached the BO, retrying the ioctl sends another DETACH command. The firmware rejects the redundant detach, and the driver handles the failure here in the cmd->result check. This again skips clearing abo->assigned_hwctx, cementing the desynchronization and leaving the BO permanently stuck as "attached". [Severity: High] This is a pre-existing issue, but does dropping the reference to the BO after the goto put_cmd; jump allow a use-after-free and hardware page fault? When aie2_hwctx_cfg_debug_bo() jumps to put_cmd, it falls through to put_obj: put_obj: amdxdna_gem_put_obj(abo); return ret; The reference to the BO is completely dropped before returning, meaning the kernel does not artificially keep the BO alive while it remains attached to the hardware context. If user space closes the BO handle while it is attached, the BO is destroyed: drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_gem_dev_obj_free() { ... if (abo->pinned) amdxdna_gem_unpin(abo); amdxdna_gem_vunmap(abo); amdxdna_gem_heap_free(abo); drm_gem_object_release(gobj); amdxdna_gem_destroy_obj(abo); } The BO is freed and unmapped from the IOMMU without checking abo->assigned_hwctx or sending a DETACH command to the hardware. The hardware context remains configured to write debug data to the freed memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
