Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible

[sashiko-bot@kernel.org]

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 <taimuraz@kaitmazov.com>
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/20260826195751.358629-1-taimuraz@kaitmazov.com?part=1