[PATCH] accel/amdxdna: fix NULL deref and GEM object leak in error paths

[PATCH] accel/amdxdna: fix NULL deref and GEM object leak in error paths

[Deniz Aydogan]

amdxdna_cmd_submit() does not set job->cmd_bo for internal driver
commands that pass AMDXDNA_INVALID_BO_HANDLE. If arg BO lookup fails
after that, the cmd_put cleanup calls amdxdna_gem_put_obj(NULL) which
dereferences a NULL pointer.

Separately, amdxdna_cmd_set_error() acquires a GEM reference via
amdxdna_gem_get_obj() when handling a chained command, but leaks it
when the subsequent amdxdna_gem_vmap() returns NULL.

Add a NULL check in the submit cleanup path and release the GEM
reference on vmap failure.

Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 31a414c3f..c0444454a 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -183,8 +183,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 		if (!abo)
 			return -EINVAL;
 		cmd = amdxdna_gem_vmap(abo);
-		if (!cmd)
+		if (!cmd) {
+			amdxdna_gem_put_obj(abo);
 			return -ENOMEM;
+		}
 	}
 
 	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));
@@ -676,7 +678,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
 put_bos:
 	amdxdna_arg_bos_put(job);
 cmd_put:
-	amdxdna_gem_put_obj(job->cmd_bo);
+	if (job->cmd_bo)
+		amdxdna_gem_put_obj(job->cmd_bo);
 free_job:
 	if (job->mm)
 		mmdrop(job->mm);
-- 
2.55.0

Re: [PATCH] accel/amdxdna: fix NULL deref and GEM object leak in error paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for NULL pointer dereference of `job->cmd_bo` during internal driver command cleanup.
--

commit 85570ac4a3fda867670c08161dee7f7bfee5d853
Author: Deniz Aydogan <denizaydogan1902@gmail.com>

accel/amdxdna: fix NULL deref and GEM object leak in error paths

amdxdna_cmd_submit() does not set job->cmd_bo for internal driver
commands that pass AMDXDNA_INVALID_BO_HANDLE. If arg BO lookup fails
after that, the cmd_put cleanup calls amdxdna_gem_put_obj(NULL) which
dereferences a NULL pointer.

Separately, amdxdna_cmd_set_error() acquires a GEM reference via
amdxdna_gem_get_obj() when handling a chained command, but leaks it
when the subsequent amdxdna_gem_vmap() returns NULL.

Add a NULL check in the submit cleanup path and release the GEM
reference on vmap failure.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 31a414c3f0d96..c0444454afe6a 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -676,7 +678,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
>  put_bos:
>  	amdxdna_arg_bos_put(job);
>  cmd_put:
> -	amdxdna_gem_put_obj(job->cmd_bo);
> +	if (job->cmd_bo)
> +		amdxdna_gem_put_obj(job->cmd_bo);

[Severity: High]
This isn't a bug introduced by this patch, but does the same unconditional NULL
pointer dereference occur in the normal job completion cleanup path?

If userspace issues a configuration IOCTL (such as DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF)
that submits an internal driver command, job->cmd_bo is deliberately left as
NULL during submission.

When the job completes, the DRM scheduler invokes aie2_sched_job_free() ->
aie2_job_release() -> amdxdna_sched_job_cleanup(), which unconditionally
calls amdxdna_gem_put_obj():

drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_sched_job_cleanup() {
    ...
    amdxdna_pm_suspend_put(job->hwctx->client->xdna);
    amdxdna_arg_bos_put(job);
    amdxdna_gem_put_obj(job->cmd_bo);
    dma_fence_put(job->fence);
    ...
}

Will this cause a kernel panic if job->cmd_bo is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829080016.10002-1-denizaydogan1902@gmail.com?part=1

[PATCH v2] accel/amdxdna: fix NULL deref and GEM object leak in error paths

[Deniz Aydogan]

amdxdna_cmd_submit() does not set job->cmd_bo for internal driver
commands that pass AMDXDNA_INVALID_BO_HANDLE. When such a job hits
an error or completes normally, both the submit error path and
amdxdna_sched_job_cleanup() call amdxdna_gem_put_obj(job->cmd_bo)
unconditionally, dereferencing NULL.

Separately, amdxdna_cmd_set_error() acquires a GEM reference via
amdxdna_gem_get_obj() when handling a chained command, but leaks it
when the subsequent amdxdna_gem_vmap() returns NULL.

Guard all three amdxdna_gem_put_obj(job->cmd_bo) call sites with a
NULL check and release the GEM reference on vmap failure.

Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 31a414c3f..a806702ec 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -183,8 +183,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 		if (!abo)
 			return -EINVAL;
 		cmd = amdxdna_gem_vmap(abo);
-		if (!cmd)
+		if (!cmd) {
+			amdxdna_gem_put_obj(abo);
 			return -ENOMEM;
+		}
 	}
 
 	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));
@@ -575,7 +577,8 @@ void amdxdna_sched_job_cleanup(struct amdxdna_sched_job *job)
 	trace_amdxdna_debug_point(job->hwctx->name, job->seq, "job release");
 	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
 	amdxdna_arg_bos_put(job);
-	amdxdna_gem_put_obj(job->cmd_bo);
+	if (job->cmd_bo)
+		amdxdna_gem_put_obj(job->cmd_bo);
 	dma_fence_put(job->fence);
 	mmdrop(job->mm);
 }
@@ -676,7 +679,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
 put_bos:
 	amdxdna_arg_bos_put(job);
 cmd_put:
-	amdxdna_gem_put_obj(job->cmd_bo);
+	if (job->cmd_bo)
+		amdxdna_gem_put_obj(job->cmd_bo);
 free_job:
 	if (job->mm)
 		mmdrop(job->mm);
-- 
2.55.0

Re: [PATCH v2] accel/amdxdna: fix NULL deref and GEM object leak in error paths

[Lizhi Hou]

On 8/29/26 01:56, Deniz Aydogan wrote:
> amdxdna_cmd_submit() does not set job->cmd_bo for internal driver
> commands that pass AMDXDNA_INVALID_BO_HANDLE. When such a job hits
> an error or completes normally, both the submit error path and
> amdxdna_sched_job_cleanup() call amdxdna_gem_put_obj(job->cmd_bo)
> unconditionally, dereferencing NULL.
>
> Separately, amdxdna_cmd_set_error() acquires a GEM reference via
> amdxdna_gem_get_obj() when handling a chained command, but leaks it
> when the subsequent amdxdna_gem_vmap() returns NULL.
>
> Guard all three amdxdna_gem_put_obj(job->cmd_bo) call sites with a
> NULL check and release the GEM reference on vmap failure.
>
> Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
> Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 31a414c3f..a806702ec 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -183,8 +183,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   		if (!abo)
>   			return -EINVAL;
>   		cmd = amdxdna_gem_vmap(abo);
> -		if (!cmd)
> +		if (!cmd) {
> +			amdxdna_gem_put_obj(abo);
>   			return -ENOMEM;
This is fixed by: 
https://lore.kernel.org/all/17811ffc-f33b-b3ac-865b-18138239e0dd@amd.com/
> +		}
>   	}
>   
>   	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));
> @@ -575,7 +577,8 @@ void amdxdna_sched_job_cleanup(struct amdxdna_sched_job *job)
>   	trace_amdxdna_debug_point(job->hwctx->name, job->seq, "job release");
>   	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
>   	amdxdna_arg_bos_put(job);
> -	amdxdna_gem_put_obj(job->cmd_bo);
> +	if (job->cmd_bo)
> +		amdxdna_gem_put_obj(job->cmd_bo);

The current code will not crash because drm_gem_object_put will check 
the if the obj pointer is NULL.

And I agree it is more robuster to check job->cmd_bo. Could you remove 
the "Fixes" tag and previous leak fix?

Thanks

Lizhi

>   	dma_fence_put(job->fence);
>   	mmdrop(job->mm);
>   }
> @@ -676,7 +679,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
>   put_bos:
>   	amdxdna_arg_bos_put(job);
>   cmd_put:
> -	amdxdna_gem_put_obj(job->cmd_bo);
> +	if (job->cmd_bo)
> +		amdxdna_gem_put_obj(job->cmd_bo);
>   free_job:
>   	if (job->mm)
>   		mmdrop(job->mm);

[PATCH v2] accel/amdxdna: add NULL check for job->cmd_bo

[Deniz Aydogan]

amdxdna_cmd_submit() leaves job->cmd_bo as NULL for internal driver
commands that use AMDXDNA_INVALID_BO_HANDLE. The cleanup paths in
amdxdna_cmd_submit() error handling and amdxdna_sched_job_cleanup()
call amdxdna_gem_put_obj(job->cmd_bo) unconditionally.

Although drm_gem_object_put() handles NULL, adding an explicit check
makes the code more robust and easier to reason about.

Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 31a414c3f..21f0560e6 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -575,7 +575,8 @@ void amdxdna_sched_job_cleanup(struct amdxdna_sched_job *job)
 	trace_amdxdna_debug_point(job->hwctx->name, job->seq, "job release");
 	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
 	amdxdna_arg_bos_put(job);
-	amdxdna_gem_put_obj(job->cmd_bo);
+	if (job->cmd_bo)
+		amdxdna_gem_put_obj(job->cmd_bo);
 	dma_fence_put(job->fence);
 	mmdrop(job->mm);
 }
@@ -676,7 +677,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
 put_bos:
 	amdxdna_arg_bos_put(job);
 cmd_put:
-	amdxdna_gem_put_obj(job->cmd_bo);
+	if (job->cmd_bo)
+		amdxdna_gem_put_obj(job->cmd_bo);
 free_job:
 	if (job->mm)
 		mmdrop(job->mm);
-- 
2.55.0