[PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client

[PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client

[Lizhi Hou]

Closing a BO handle clears abo->client, while the underlying GEM object
may remain alive due to internal kernel references. As a result, code
executed after the BO handle is closed may dereference a NULL abo->client
pointer.

Remove accesses to abo->client from code paths that may execute after the
BO handle has been closed.

Fixes: d76856beb4a4 ("accel/amdxdna: Refactor GEM BO handling and add helper APIs for address retrieval")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_message.c |  4 ++--
 drivers/accel/amdxdna/amdxdna_gem.c  | 11 +++++++++--
 drivers/accel/amdxdna/amdxdna_gem.h  | 11 +++++++++--
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
index c4b364801cc0..dfe0fbdf066d 100644
--- a/drivers/accel/amdxdna/aie2_message.c
+++ b/drivers/accel/amdxdna/aie2_message.c
@@ -840,7 +840,7 @@ static struct aie2_exec_msg_ops npu_exec_message_ops = {
 static int aie2_init_exec_req(void *req, struct amdxdna_gem_obj *cmd_abo,
 			      size_t *size, u32 *msg_op)
 {
-	struct amdxdna_dev *xdna = cmd_abo->client->xdna;
+	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(cmd_abo)->dev);
 	int ret;
 	u32 op;
 
@@ -874,7 +874,7 @@ static int
 aie2_cmdlist_fill_slot(void *slot, struct amdxdna_gem_obj *cmd_abo,
 		       size_t *size, u32 *cmd_op)
 {
-	struct amdxdna_dev *xdna = cmd_abo->client->xdna;
+	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(cmd_abo)->dev);
 	int ret;
 	u32 op;
 
diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1275f91ca705..4628a2787265 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -198,6 +198,7 @@ amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
  */
 void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
 {
+	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(NULL);
 	int ret;
 
@@ -210,7 +211,7 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
 	if (!abo->mem.kva) {
 		ret = drm_gem_vmap(to_gobj(abo), &map);
 		if (ret)
-			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
+			XDNA_ERR(xdna, "Vmap bo failed, ret %d", ret);
 		else
 			abo->mem.kva = map.vaddr;
 	}
@@ -354,7 +355,13 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
 	unsigned long nr_pages;
 	int ret;
 
-	if (!amdxdna_pasid_on(abo->client)) {
+	/*
+	 * When PASID is off, amdxdna_gem_obj_open() called amdxdna_dma_map_bo()
+	 * and mem.dma_addr is valid; use the DMA address directly and skip HMM.
+	 * Avoid dereferencing abo->client which may be NULL (cleared in close())
+	 * while internal kernel references are still held.
+	 */
+	if (abo->mem.dma_addr != AMDXDNA_INVALID_ADDR) {
 		/* Need to set uva for heap uva validation */
 		abo->mem.uva = addr;
 		return 0;
diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
index a35d2f15d32c..1e90e32bf3cd 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.h
+++ b/drivers/accel/amdxdna/amdxdna_gem.h
@@ -88,12 +88,19 @@ u64 amdxdna_gem_dev_addr(struct amdxdna_gem_obj *abo);
 
 static inline u64 amdxdna_dev_bo_offset(struct amdxdna_gem_obj *abo)
 {
-	return amdxdna_gem_dev_addr(abo) - abo->client->xdna->dev_info->dev_mem_base;
+	return amdxdna_gem_dev_addr(abo) - to_xdna_dev(to_gobj(abo)->dev)->dev_info->dev_mem_base;
 }
 
 static inline u64 amdxdna_obj_dma_addr(struct amdxdna_gem_obj *abo)
 {
-	return amdxdna_pasid_on(abo->client) ? amdxdna_gem_uva(abo) : abo->mem.dma_addr;
+	/*
+	 * amdxdna_gem_obj_open() calls amdxdna_dma_map_bo() only when PASID is
+	 * off, leaving mem.dma_addr at AMDXDNA_INVALID_ADDR when PASID is on.
+	 * Avoid dereferencing abo->client, which is cleared to NULL by
+	 * amdxdna_gem_obj_close() while internal kernel references remain.
+	 */
+	return (abo->mem.dma_addr != AMDXDNA_INVALID_ADDR) ?
+		abo->mem.dma_addr : amdxdna_gem_uva(abo);
 }
 
 void amdxdna_umap_put(struct amdxdna_umap *mapp);
-- 
2.34.1

Re: [PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client

[sashiko-bot]

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

Pre-existing issues:
- [High] Double put of GEM object on the error path of amdxdna_insert_pages()
- [High] Missing self-import check in amdxdna_gem_prime_import causes duplicate GEM objects
--

--- Patch [1]: [PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client ---
commit d74333f9f2549a77933710d6e3e66ba616b070a4
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix potential NULL pointer dereference of abo->client

This commit fixes a potential NULL pointer dereference when a BO handle is
closed but the underlying GEM object remains alive. It avoids accessing
abo->client in code paths executing after the handle closure.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index c562d3ae6f3f5..1adb7e57f50ce 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
amdxdna_insert_pages() while reviewing this file:

        ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages,
                              &num_pages);
        if (ret) {
                XDNA_ERR(xdna, "Failed insert pages %d", ret);
                vma->vm_ops->close(vma);
                return ret;
        }

Does calling close() here prematurely drop the GEM object reference?

When userspace calls mmap() on an AMDXDNA BO, drm_gem_mmap_obj() acquires a
reference to the GEM object and calls the driver's .mmap callback, which
eventually calls amdxdna_insert_pages().

If vm_insert_pages() fails, the driver explicitly calls
vma->vm_ops->close(vma), which drops the GEM object reference. The error is
then returned back to drm_gem_mmap_obj(), which executes its own error path
and calls drm_gem_object_put() a second time.

Could this double-put cause an underflow of the object's reference count and
lead to a use-after-free of the GEM object?


[Severity: High]
This is also a pre-existing issue, but does amdxdna_gem_prime_import() miss
a self-import check?

struct drm_gem_object *
amdxdna_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf)
{
        struct dma_buf_attachment *attach;
        struct amdxdna_gem_obj *abo;
        struct drm_gem_object *gobj;
        struct sg_table *sgt;
        int ret;

        get_dma_buf(dma_buf);

        attach = dma_buf_attach(dma_buf, dev->dev);

Since the DRM core defers to the driver's .gem_prime_import callback, and
this function unconditionally creates a new amdxdna_gem_obj without checking
if the DMA-BUF was originally exported by the same device, could this create
duplicate GEM objects for the same memory?

It looks like unprivileged userspace could export a BO via PRIME to a file
descriptor and import it back into the same device, which might break handle
de-duplication and cause state tracking inconsistencies.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707201556.562191-1-lizhi.hou@amd.com?part=1

Re: [PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client

[Max Zhen]

On 7/7/2026 Tue 13:15, Lizhi Hou wrote:
> Closing a BO handle clears abo->client, while the underlying GEM object
> may remain alive due to internal kernel references. As a result, code
> executed after the BO handle is closed may dereference a NULL abo->client
> pointer.
> 
> Remove accesses to abo->client from code paths that may execute after the
> BO handle has been closed.
> 
> Fixes: d76856beb4a4 ("accel/amdxdna: Refactor GEM BO handling and add helper APIs for address retrieval")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Max Zhen <max.zhen@amd.com>
> ---
>   drivers/accel/amdxdna/aie2_message.c |  4 ++--
>   drivers/accel/amdxdna/amdxdna_gem.c  | 11 +++++++++--
>   drivers/accel/amdxdna/amdxdna_gem.h  | 11 +++++++++--
>   3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
> index c4b364801cc0..dfe0fbdf066d 100644
> --- a/drivers/accel/amdxdna/aie2_message.c
> +++ b/drivers/accel/amdxdna/aie2_message.c
> @@ -840,7 +840,7 @@ static struct aie2_exec_msg_ops npu_exec_message_ops = {
>   static int aie2_init_exec_req(void *req, struct amdxdna_gem_obj *cmd_abo,
>   			      size_t *size, u32 *msg_op)
>   {
> -	struct amdxdna_dev *xdna = cmd_abo->client->xdna;
> +	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(cmd_abo)->dev);
>   	int ret;
>   	u32 op;
>   
> @@ -874,7 +874,7 @@ static int
>   aie2_cmdlist_fill_slot(void *slot, struct amdxdna_gem_obj *cmd_abo,
>   		       size_t *size, u32 *cmd_op)
>   {
> -	struct amdxdna_dev *xdna = cmd_abo->client->xdna;
> +	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(cmd_abo)->dev);
>   	int ret;
>   	u32 op;
>   
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 1275f91ca705..4628a2787265 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -198,6 +198,7 @@ amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
>    */
>   void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
>   {
> +	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>   	struct iosys_map map = IOSYS_MAP_INIT_VADDR(NULL);
>   	int ret;
>   
> @@ -210,7 +211,7 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
>   	if (!abo->mem.kva) {
>   		ret = drm_gem_vmap(to_gobj(abo), &map);
>   		if (ret)
> -			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
> +			XDNA_ERR(xdna, "Vmap bo failed, ret %d", ret);
>   		else
>   			abo->mem.kva = map.vaddr;
>   	}
> @@ -354,7 +355,13 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
>   	unsigned long nr_pages;
>   	int ret;
>   
> -	if (!amdxdna_pasid_on(abo->client)) {
> +	/*
> +	 * When PASID is off, amdxdna_gem_obj_open() called amdxdna_dma_map_bo()
> +	 * and mem.dma_addr is valid; use the DMA address directly and skip HMM.
> +	 * Avoid dereferencing abo->client which may be NULL (cleared in close())
> +	 * while internal kernel references are still held.
> +	 */
> +	if (abo->mem.dma_addr != AMDXDNA_INVALID_ADDR) {
>   		/* Need to set uva for heap uva validation */
>   		abo->mem.uva = addr;
>   		return 0;
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
> index a35d2f15d32c..1e90e32bf3cd 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.h
> +++ b/drivers/accel/amdxdna/amdxdna_gem.h
> @@ -88,12 +88,19 @@ u64 amdxdna_gem_dev_addr(struct amdxdna_gem_obj *abo);
>   
>   static inline u64 amdxdna_dev_bo_offset(struct amdxdna_gem_obj *abo)
>   {
> -	return amdxdna_gem_dev_addr(abo) - abo->client->xdna->dev_info->dev_mem_base;
> +	return amdxdna_gem_dev_addr(abo) - to_xdna_dev(to_gobj(abo)->dev)->dev_info->dev_mem_base;
>   }
>   
>   static inline u64 amdxdna_obj_dma_addr(struct amdxdna_gem_obj *abo)
>   {
> -	return amdxdna_pasid_on(abo->client) ? amdxdna_gem_uva(abo) : abo->mem.dma_addr;
> +	/*
> +	 * amdxdna_gem_obj_open() calls amdxdna_dma_map_bo() only when PASID is
> +	 * off, leaving mem.dma_addr at AMDXDNA_INVALID_ADDR when PASID is on.
> +	 * Avoid dereferencing abo->client, which is cleared to NULL by
> +	 * amdxdna_gem_obj_close() while internal kernel references remain.
> +	 */
> +	return (abo->mem.dma_addr != AMDXDNA_INVALID_ADDR) ?
> +		abo->mem.dma_addr : amdxdna_gem_uva(abo);
>   }
>   
>   void amdxdna_umap_put(struct amdxdna_umap *mapp);