[PATCH V3 1/3] accel/amdxdna: Fix amdxdna_client lifetime race during device removal

[PATCH V3 1/3] accel/amdxdna: Fix amdxdna_client lifetime race during device removal

[Lizhi Hou]

In amdxdna_remove(), all amdxdna_client structures are freed after
calling drm_dev_unplug(). However, drm_dev_unplug() does not force
existing file descriptors to be closed, so amdxdna_drm_close() may be
called after amdxdna_remove() has completed.

As a result, accessing client->pid for debug output in
amdxdna_drm_close() can lead to a use-after-free, since the access is
not protected by drm_dev_enter().

Fix this by decoupling hardware teardown from client cleanup.
amdxdna_remove() only performs hardware-related cleanup, while
per-client resources are released from amdxdna_drm_close() when the
corresponding file is closed.

Fixes: be462c97b7df ("accel/amdxdna: Add hardware context")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_pci_drv.c | 23 ++++++++++-------------
 drivers/accel/amdxdna/amdxdna_pci_drv.h |  1 +
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
index 65489bb3f2b0..470bf4fc744b 100644
--- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
+++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
@@ -138,9 +138,11 @@ static int amdxdna_drm_open(struct drm_device *ddev, struct drm_file *filp)
 		    xdna->dev_info->dev_heap_max_size);
 	mutex_init(&client->mm_lock);
 
+	mutex_lock(&xdna->client_lock);
 	mutex_lock(&xdna->dev_lock);
 	list_add_tail(&client->node, &xdna->client_list);
 	mutex_unlock(&xdna->dev_lock);
+	mutex_unlock(&xdna->client_lock);
 
 	filp->driver_priv = client;
 	client->filp = filp;
@@ -174,18 +176,14 @@ static void amdxdna_drm_close(struct drm_device *ddev, struct drm_file *filp)
 {
 	struct amdxdna_client *client = filp->driver_priv;
 	struct amdxdna_dev *xdna = to_xdna_dev(ddev);
-	int idx;
 
 	XDNA_DBG(xdna, "closing pid %d", client->pid);
 
-	if (!drm_dev_enter(&xdna->ddev, &idx))
-		return;
-
+	mutex_lock(&xdna->client_lock);
 	mutex_lock(&xdna->dev_lock);
 	amdxdna_client_cleanup(client);
 	mutex_unlock(&xdna->dev_lock);
-
-	drm_dev_exit(idx);
+	mutex_unlock(&xdna->client_lock);
 }
 
 static int amdxdna_drm_get_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
@@ -372,6 +370,7 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENODEV;
 
 	drmm_mutex_init(ddev, &xdna->dev_lock);
+	drmm_mutex_init(ddev, &xdna->client_lock);
 	init_rwsem(&xdna->notifier_lock);
 	INIT_LIST_HEAD(&xdna->client_list);
 	pci_set_drvdata(pdev, xdna);
@@ -442,18 +441,16 @@ static void amdxdna_remove(struct pci_dev *pdev)
 	drm_dev_unplug(&xdna->ddev);
 	amdxdna_sysfs_fini(xdna);
 
+	mutex_lock(&xdna->client_lock);
 	mutex_lock(&xdna->dev_lock);
-	client = list_first_entry_or_null(&xdna->client_list,
-					  struct amdxdna_client, node);
-	while (client) {
-		amdxdna_client_cleanup(client);
-
-		client = list_first_entry_or_null(&xdna->client_list,
-						  struct amdxdna_client, node);
+	list_for_each_entry(client, &xdna->client_list, node) {
+		amdxdna_hwctx_remove_all(client);
+		amdxdna_sva_fini(client);
 	}
 
 	xdna->dev_info->ops->fini(xdna);
 	mutex_unlock(&xdna->dev_lock);
+	mutex_unlock(&xdna->client_lock);
 
 	amdxdna_iommu_fini(xdna);
 }
diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.h b/drivers/accel/amdxdna/amdxdna_pci_drv.h
index 34271c14d359..a997d27a504d 100644
--- a/drivers/accel/amdxdna/amdxdna_pci_drv.h
+++ b/drivers/accel/amdxdna/amdxdna_pci_drv.h
@@ -120,6 +120,7 @@ struct amdxdna_dev {
 
 	struct mutex			dev_lock; /* per device lock */
 	struct list_head		client_list;
+	struct mutex			client_lock; /* client_list */
 	struct amdxdna_fw_ver		fw_ver;
 	struct rw_semaphore		notifier_lock; /* for mmu notifier*/
 	struct workqueue_struct		*notifier_wq;
-- 
2.34.1

[PATCH V3 2/3] accel/amdxdna: Fix notifier_wq lifetime race during device removal

[Lizhi Hou]

amdxdna_remove() destroys notifier_wq. If amdxdna_gem_obj_free() is
called after device removal, it may attempt to flush notifier_wq,
resulting in a use-after-free.

Fix the race by allocating notifier_wq with
drmm_alloc_ordered_workqueue(), so its lifetime is managed by DRM and
remains valid until all managed resources are released.

Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_pci_drv.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
index 470bf4fc744b..b943c06a451e 100644
--- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
+++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
@@ -389,9 +389,9 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (ret)
 		return ret;
 
-	xdna->notifier_wq = alloc_ordered_workqueue("notifier_wq", WQ_MEM_RECLAIM);
-	if (!xdna->notifier_wq) {
-		ret = -ENOMEM;
+	xdna->notifier_wq = drmm_alloc_ordered_workqueue(ddev, "notifier_wq", WQ_MEM_RECLAIM);
+	if (IS_ERR(xdna->notifier_wq)) {
+		ret = PTR_ERR(xdna->notifier_wq);
 		goto iommu_fini;
 	}
 
@@ -400,7 +400,7 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mutex_unlock(&xdna->dev_lock);
 	if (ret) {
 		XDNA_ERR(xdna, "Hardware init failed, ret %d", ret);
-		goto destroy_notifier_wq;
+		goto iommu_fini;
 	}
 
 	ret = amdxdna_sysfs_init(xdna);
@@ -424,8 +424,6 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mutex_lock(&xdna->dev_lock);
 	xdna->dev_info->ops->fini(xdna);
 	mutex_unlock(&xdna->dev_lock);
-destroy_notifier_wq:
-	destroy_workqueue(xdna->notifier_wq);
 iommu_fini:
 	amdxdna_iommu_fini(xdna);
 	return ret;
@@ -436,8 +434,6 @@ static void amdxdna_remove(struct pci_dev *pdev)
 	struct amdxdna_dev *xdna = pci_get_drvdata(pdev);
 	struct amdxdna_client *client;
 
-	destroy_workqueue(xdna->notifier_wq);
-
 	drm_dev_unplug(&xdna->ddev);
 	amdxdna_sysfs_fini(xdna);
 
-- 
2.34.1

[PATCH V3 3/3] accel/amdxdna: Fix iommu domain lifetime race during device removal

[Lizhi Hou]

When force_iova mode is enabled, amdxdna_remove() frees xdna->domain. If
amdxdna_gem_obj_free() is called after device removal, it may attempt to
access xdna->domain, resulting in a use-after-free.

Fix the race by adding freeing xdna->domain as a managed release action,
so its lifetime is managed by DRM and remains valid until all managed
resources are released.

Fixes: ece3e8980907 ("accel/amdxdna: Allow forcing IOVA-based DMA via module parameter")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_iommu.c | 43 ++++++++++++++++++---------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_iommu.c b/drivers/accel/amdxdna/amdxdna_iommu.c
index eff00131d0f8..4f245b969eef 100644
--- a/drivers/accel/amdxdna/amdxdna_iommu.c
+++ b/drivers/accel/amdxdna/amdxdna_iommu.c
@@ -4,6 +4,7 @@
  */
 
 #include <drm/amdxdna_accel.h>
+#include <drm/drm_managed.h>
 #include <linux/iommu.h>
 #include <linux/iova.h>
 
@@ -153,10 +154,30 @@ void amdxdna_iommu_free(struct amdxdna_dev *xdna, size_t size,
 	free_pages((unsigned long)cpu_addr, get_order(size));
 }
 
+static void amdxdna_cleanup_force_iova(struct drm_device *dev, void *res)
+{
+	struct amdxdna_dev *xdna = to_xdna_dev(dev);
+
+	if (xdna->domain) {
+		iommu_detach_group(xdna->domain, xdna->group);
+		put_iova_domain(&xdna->iovad);
+		iova_cache_put();
+		iommu_domain_free(xdna->domain);
+	}
+
+	iommu_group_put(xdna->group);
+}
+
+void amdxdna_iommu_fini(struct amdxdna_dev *xdna)
+{
+	if (xdna->group && !xdna->domain)
+		iommu_group_put(xdna->group);
+}
+
 int amdxdna_iommu_init(struct amdxdna_dev *xdna)
 {
 	unsigned long order;
-	int ret;
+	int ret = 0;
 
 	xdna->group = iommu_group_get(xdna->ddev.dev);
 	if (!xdna->group || !force_iova)
@@ -182,8 +203,14 @@ int amdxdna_iommu_init(struct amdxdna_dev *xdna)
 	if (ret)
 		goto put_iova;
 
+	ret = drmm_add_action(&xdna->ddev, amdxdna_cleanup_force_iova, NULL);
+	if (ret)
+		goto detach_group;
+
 	return 0;
 
+detach_group:
+	iommu_detach_group(xdna->domain, xdna->group);
 put_iova:
 	put_iova_domain(&xdna->iovad);
 	iova_cache_put();
@@ -191,20 +218,8 @@ int amdxdna_iommu_init(struct amdxdna_dev *xdna)
 	iommu_domain_free(xdna->domain);
 put_group:
 	iommu_group_put(xdna->group);
+	xdna->group = NULL;
 	xdna->domain = NULL;
 
 	return ret;
 }
-
-void amdxdna_iommu_fini(struct amdxdna_dev *xdna)
-{
-	if (xdna->domain) {
-		iommu_detach_group(xdna->domain, xdna->group);
-		put_iova_domain(&xdna->iovad);
-		iova_cache_put();
-		iommu_domain_free(xdna->domain);
-	}
-
-	if (xdna->group)
-		iommu_group_put(xdna->group);
-}
-- 
2.34.1

Re: [PATCH V3 3/3] accel/amdxdna: Fix iommu domain lifetime race during device removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Delaying iommu_detach_group() to the DRM release action prevents driver rebind while the DRM device is kept open by userspace.
--

commit 873614c426570f83392d242f0aa4d39f76d04bd7
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix iommu domain lifetime race during device removal

This commit fixes a use-after-free race during device removal by moving
the freeing of the iommu domain to a drm managed release action. This
ensures the domain lifetime is maintained by the DRM subsystem until all
managed resources are released.

> diff --git a/drivers/accel/amdxdna/amdxdna_iommu.c b/drivers/accel/amdxdna/amdxdna_iommu.c
> index eff00131d0f80..4f245b969eef1 100644
> --- a/drivers/accel/amdxdna/amdxdna_iommu.c
> +++ b/drivers/accel/amdxdna/amdxdna_iommu.c

[ ... ]

> @@ -153,10 +154,30 @@ void amdxdna_iommu_free(struct amdxdna_dev *xdna, size_t size,
>  	free_pages((unsigned long)cpu_addr, get_order(size));
>  }
>  
> +static void amdxdna_cleanup_force_iova(struct drm_device *dev, void *res)
> +{
> +	struct amdxdna_dev *xdna = to_xdna_dev(dev);
> +
> +	if (xdna->domain) {
> +		iommu_detach_group(xdna->domain, xdna->group);

[Severity: High]
Does delaying iommu_detach_group() to the drm managed release action
prevent driver rebind if userspace holds the DRM file descriptor open?

If a user holds the fd open after the device is unbound via sysfs, the
hardware IOMMU group will stay attached to the domain until the file is
finally closed. If the device is subsequently re-bound, or bound to
vfio-pci, iommu_attach_group() in the new probe would return -EBUSY
because the group is already attached.

Should iommu_detach_group() execute synchronously during amdxdna_remove(),
while only deferring iommu_domain_free() and the IOVA state to this release
action?

> +		put_iova_domain(&xdna->iovad);
> +		iova_cache_put();
> +		iommu_domain_free(xdna->domain);
> +	}
> +
> +	iommu_group_put(xdna->group);
> +}
> +
> +void amdxdna_iommu_fini(struct amdxdna_dev *xdna)
> +{
> +	if (xdna->group && !xdna->domain)
> +		iommu_group_put(xdna->group);
> +}

[Severity: High]
Because amdxdna_iommu_fini() now skips putting the IOMMU group when a
domain exists, does this intentionally defer releasing the group until the
drmm release action runs?

This contributes to the same rebind failure mentioned above, as the
resource is locked for the duration of the open userspace file descriptor.

[ ... ]

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

Re: [PATCH V3 2/3] accel/amdxdna: Fix notifier_wq lifetime race during device removal

[sashiko-bot]

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

Pre-existing issues:
- [High] Asynchronous work item `amdxdna_hmm_unreg_work` dereferences a potentially freed `vm_area_struct` during MMU notifier release.
--

commit 9bd885c56a90bdf89e66e4a081996397ef692196
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix notifier_wq lifetime race during device removal
    
This commit fixes a use-after-free race during device removal by converting
notifier_wq to use drmm_alloc_ordered_workqueue. This ensures the workqueue
lifetime is managed by DRM and remains valid until resources are released.

> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index 470bf4fc744bf..b943c06a451ef 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
[ ... ]
> @@ -389,9 +389,9 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (ret)
>  		return ret;
>  
> -	xdna->notifier_wq = alloc_ordered_workqueue("notifier_wq", WQ_MEM_RECLAIM);
> -	if (!xdna->notifier_wq) {
> -		ret = -ENOMEM;
> +	xdna->notifier_wq = drmm_alloc_ordered_workqueue(ddev, "notifier_wq", WQ_MEM_RECLAIM);
> +	if (IS_ERR(xdna->notifier_wq)) {
> +		ret = PTR_ERR(xdna->notifier_wq);

[Severity: High]
This is a pre-existing issue, but can the work items queued to this
workqueue trigger a use-after-free on a vm_area_struct?

Looking at drivers/accel/amdxdna/amdxdna_gem.c, when handling an
MMU_NOTIFY_UNMAP event, the hmm_unreg_work is queued to this notifier_wq
to execute asynchronously. The worker eventually calls amdxdna_umap_release()
which dereferences the vma:

if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping)
        mapping_clear_unevictable(vma->vm_file->f_mapping);

Since the work item is asynchronous, couldn't the core memory management
subsystem tear down the mapping and free the vm_area_struct before
amdxdna_umap_release() executes, leading to a use-after-free when accessing
vma->vm_file?

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

Re: [PATCH V3 1/3] accel/amdxdna: Fix amdxdna_client lifetime race during device removal

[sashiko-bot]

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

New issues:
- [Medium] Ignored return value of `drmm_mutex_init()` for `xdna->client_lock` can result in using a destroyed mutex.

Pre-existing issues:
- [High] Missing drm_dev_enter() in amdxdna_drm_open() and ioctls allows races with device unplug, leading to use-after-free and hardware access on torn-down devices.
- [High] Unchecked return value of `init_srcu_struct()` allows execution with an uninitialized SRCU context.
--

commit 16ed88a14ca5cdcb72cc707cc054f2d2b9526777
Author: Lizhi Hou <lizhi.hou@amd.com>
accel/amdxdna: Fix amdxdna_client lifetime race during device removal

This patch decouples hardware teardown from client cleanup in the amdxdna
driver. It restricts amdxdna_remove() to hardware-related cleanup and 
defers releasing per-client resources to amdxdna_drm_close() when the file
is actually closed.

> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index 65489bb3f2b07..470bf4fc744bf 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> @@ -138,9 +138,11 @@ static int amdxdna_drm_open(struct drm_device *ddev, struct drm_file *filp)
>  		    xdna->dev_info->dev_heap_max_size);
>  	mutex_init(&client->mm_lock);
>  
> +	mutex_lock(&xdna->client_lock);
>  	mutex_lock(&xdna->dev_lock);
>  	list_add_tail(&client->node, &xdna->client_list);
>  	mutex_unlock(&xdna->dev_lock);
> +	mutex_unlock(&xdna->client_lock);

[Severity: High]
This is a pre-existing issue, but does amdxdna_drm_open() need to check the
return value of init_srcu_struct() called just prior to this block? 

If the internal allocation fails and returns -ENOMEM, the SRCU context
remains uninitialized, which could lead to a NULL pointer dereference when
later using srcu_read_lock().

[Severity: High]
This is also a pre-existing issue, but should amdxdna_drm_open() and ioctls
like amdxdna_drm_get_info_ioctl() be protected by drm_dev_enter()?

Without it, an open operation could race with device unplug, successfully
bind SVA after amdxdna_remove() has processed the client list, and then
later cause a use-after-free during amdxdna_drm_close() when it attempts to
unbind SVA on a torn-down IOMMU. 

Similarly, an unprotected ioctl could access hardware state after it has
been torn down.

[ ... ]

> @@ -372,6 +370,7 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return -ENODEV;
>  
>  	drmm_mutex_init(ddev, &xdna->dev_lock);
> +	drmm_mutex_init(ddev, &xdna->client_lock);

[Severity: Medium]
Is it safe to ignore the return value of drmm_mutex_init() here? 

If the internal memory allocation for the managed cleanup action fails, the
macro will automatically destroy the mutex and return an error code. Since
amdxdna_probe() ignores this and continues, could subsequent driver
operations end up using a destroyed mutex?

>  	init_rwsem(&xdna->notifier_lock);
>  	INIT_LIST_HEAD(&xdna->client_list);
>  	pci_set_drvdata(pdev, xdna);

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