[PATCH V2] accel/amdxdna: Fix VMA access race

[PATCH V2] accel/amdxdna: Fix VMA access race

[Lizhi Hou]

aie2_populate_range() and amdxdna_umap_release() access a saved VMA
pointer that may have already been freed, leading to a potential
use-after-free.

Remove the VMA accesses from these functions to avoid the race.

Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
V2:
  fix sashiko comments.

 drivers/accel/amdxdna/aie2_ctx.c    |  2 --
 drivers/accel/amdxdna/amdxdna_gem.c | 35 ++++++++++++++---------------
 drivers/accel/amdxdna/amdxdna_gem.h |  1 -
 3 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index da89b3701f5b..3e21e2dabe82 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -1023,8 +1023,6 @@ static int aie2_populate_range(struct amdxdna_gem_obj *abo)
 	kref_get(&mapp->refcnt);
 	up_write(&xdna->notifier_lock);
 
-	XDNA_DBG(xdna, "populate memory range %lx %lx",
-		 mapp->vma->vm_start, mapp->vma->vm_end);
 	mm = mapp->notifier.mm;
 	if (!mmget_not_zero(mm)) {
 		amdxdna_umap_put(mapp);
diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 63976c3bcbe0..079914194efc 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -254,7 +254,7 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni,
 
 	xdna = to_xdna_dev(to_gobj(abo)->dev);
 	XDNA_DBG(xdna, "Invalidating range 0x%lx, 0x%lx, type %d",
-		 mapp->vma->vm_start, mapp->vma->vm_end, abo->type);
+		 mapp->range.start, mapp->range.end, abo->type);
 
 	if (!mmu_notifier_range_blockable(range))
 		return false;
@@ -284,21 +284,26 @@ static const struct mmu_interval_notifier_ops amdxdna_hmm_ops = {
 	.invalidate = amdxdna_hmm_invalidate,
 };
 
-static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
-				   struct vm_area_struct *vma)
+static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, bool unreg_all)
 {
 	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
 	struct amdxdna_umap *mapp;
 
+	if (!unreg_all) {
+		down_read(&xdna->notifier_lock);
+		mapp = list_last_entry(&abo->mem.umap_list, struct amdxdna_umap, node);
+		queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
+		mapp->unmapped = true;
+		up_read(&xdna->notifier_lock);
+
+		return;
+	}
+
 	down_read(&xdna->notifier_lock);
 	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
-		if (!vma || mapp->vma == vma) {
-			if (!mapp->unmapped) {
-				queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
-				mapp->unmapped = true;
-			}
-			if (vma)
-				break;
+		if (!mapp->unmapped) {
+			queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
+			mapp->unmapped = true;
 		}
 	}
 	up_read(&xdna->notifier_lock);
@@ -308,12 +313,9 @@ static void amdxdna_umap_release(struct kref *ref)
 {
 	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
 	struct amdxdna_gem_obj *abo = mapp->abo;
-	struct vm_area_struct *vma = mapp->vma;
 	struct amdxdna_dev *xdna;
 
 	mmu_interval_notifier_remove(&mapp->notifier);
-	if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping)
-		mapping_clear_unevictable(vma->vm_file->f_mapping);
 
 	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
 	down_write(&xdna->notifier_lock);
@@ -380,13 +382,10 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
 	mapp->range.start = vma->vm_start;
 	mapp->range.end = vma->vm_end;
 	mapp->range.default_flags = HMM_PFN_REQ_FAULT;
-	mapp->vma = vma;
 	mapp->abo = abo;
 	kref_init(&mapp->refcnt);
 
 	INIT_WORK(&mapp->hmm_unreg_work, amdxdna_hmm_unreg_work);
-	if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping)
-		mapping_set_unevictable(vma->vm_file->f_mapping);
 
 	down_write(&xdna->notifier_lock);
 	if (list_empty(&abo->mem.umap_list))
@@ -497,7 +496,7 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
 	return 0;
 
 hmm_unreg:
-	amdxdna_hmm_unregister(abo, vma);
+	amdxdna_hmm_unregister(abo, false);
 	return ret;
 }
 
@@ -619,7 +618,7 @@ static void amdxdna_gem_obj_free(struct drm_gem_object *gobj)
 	struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
 	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
 
-	amdxdna_hmm_unregister(abo, NULL);
+	amdxdna_hmm_unregister(abo, true);
 	flush_workqueue(xdna->notifier_wq);
 
 	if (abo->pinned)
diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
index a3e44c7a2395..a35d2f15d32c 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.h
+++ b/drivers/accel/amdxdna/amdxdna_gem.h
@@ -12,7 +12,6 @@
 #include "amdxdna_pci_drv.h"
 
 struct amdxdna_umap {
-	struct vm_area_struct		*vma;
 	struct mmu_interval_notifier	notifier;
 	struct hmm_range		range;
 	struct work_struct		hmm_unreg_work;
-- 
2.34.1

Re: [PATCH V2] accel/amdxdna: Fix VMA access race

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Using `list_last_entry()` for error-path cleanup in `amdxdna_hmm_unregister()` introduces severe race conditions and out-of-bounds memory accesses.
--

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

accel/amdxdna: Fix VMA access race

This commit removes VMA accesses from aie2_populate_range() and
amdxdna_umap_release() to fix a potential use-after-free where a saved VMA
pointer may have already been freed. It also updates amdxdna_hmm_unregister()
to unregister mappings based on a boolean flag instead of a VMA pointer.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> @@ -284,21 +284,26 @@ static const struct mmu_interval_notifier_ops amdxdna_hmm_ops = {
>  	.invalidate = amdxdna_hmm_invalidate,
>  };
>  
> -static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
> -				   struct vm_area_struct *vma)
> +static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, bool unreg_all)
>  {
>  	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>  	struct amdxdna_umap *mapp;
>  
> +	if (!unreg_all) {
> +		down_read(&xdna->notifier_lock);
> +		mapp = list_last_entry(&abo->mem.umap_list, struct amdxdna_umap, node);

[Severity: Critical]
Can this cause an out-of-bounds access if the list is empty?

Looking at amdxdna_hmm_register(), it can return 0 without adding an entry
to umap_list:

amdxdna_hmm_register() {
	if (!amdxdna_pasid_on(abo->client)) {
		abo->mem.uva = addr;
		return 0;
	}
	...
}

If amdxdna_insert_pages() subsequently fails in amdxdna_gem_obj_mmap(), the
error path calls amdxdna_hmm_unregister(abo, false). Since the list is empty,
would list_last_entry() evaluate to an invalid pointer offset, leading to
memory corruption when setting mapp->unmapped = true below?

Additionally, is there a race condition here with concurrent mappings?
If multiple processes mmap the same GEM object concurrently, and one
thread's amdxdna_insert_pages() fails, could it incorrectly retrieve and
unregister the other thread's mapping from the end of the list?

> +		queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
> +		mapp->unmapped = true;
> +		up_read(&xdna->notifier_lock);
> +
> +		return;
> +	}

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