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

[PATCH V3] 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>
---
V3:
  fix sashiko comments: error-path cleanup patch race
V2:
  fix sashiko comments: Use-after-free on `mapp->vma`

 drivers/accel/amdxdna/aie2_ctx.c    |  2 --
 drivers/accel/amdxdna/amdxdna_gem.c | 31 +++++++++++++++++++----------
 drivers/accel/amdxdna/amdxdna_gem.h |  1 -
 3 files changed, 21 insertions(+), 13 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..20ce304b19ef 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,15 +284,23 @@ static const struct mmu_interval_notifier_ops amdxdna_hmm_ops = {
 	.invalidate = amdxdna_hmm_invalidate,
 };
 
+static inline bool compare_range(struct amdxdna_umap *mapp,
+				 struct mm_struct *mm,
+				 unsigned long start, unsigned long end)
+{
+	return (!mapp->unmapped && mapp->notifier.mm == mm &&
+		mapp->range.start == start && mapp->range.end == end);
+}
+
 static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
 				   struct vm_area_struct *vma)
 {
 	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
 	struct amdxdna_umap *mapp;
 
-	down_read(&xdna->notifier_lock);
+	down_write(&xdna->notifier_lock);
 	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
-		if (!vma || mapp->vma == vma) {
+		if (!vma || compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) {
 			if (!mapp->unmapped) {
 				queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
 				mapp->unmapped = true;
@@ -301,19 +309,16 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
 				break;
 		}
 	}
-	up_read(&xdna->notifier_lock);
+	up_write(&xdna->notifier_lock);
 }
 
 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);
@@ -355,6 +360,15 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
 		return 0;
 	}
 
+	down_read(&xdna->notifier_lock);
+	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
+		if (compare_range(mapp, current->mm, addr, addr + len)) {
+			up_read(&xdna->notifier_lock);
+			return 0;
+		}
+	}
+	up_read(&xdna->notifier_lock);
+
 	mapp = kzalloc_obj(*mapp);
 	if (!mapp)
 		return -ENOMEM;
@@ -380,13 +394,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))
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 V3] accel/amdxdna: Fix VMA access race

[sashiko-bot]

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

Pre-existing issues:
- [High] Partial unmap or mremap of a buffer drops the MMU interval notifier for the entire mapping, leading to a silent Use-After-Free of physical pages if the remaining mapped pages are later evicted.
- [High] aie2_populate_range() permanently leaks a reference to the process's mm_struct on every successful iteration or retry.
- [High] amdxdna_umap_release() makes a blocking call before removing the object from umap_list, causing a race condition where aie2_populate_range() can acquire a use-after-free reference.
--

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

accel/amdxdna: Fix VMA access race

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.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index da89b3701f5b6..3e21e2dabe820 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);
>  

[Severity: High]
This isn't a bug introduced by this patch, but can this sequence lead to a
use-after-free or double-free?

In amdxdna_umap_release(), the blocking mmu_interval_notifier_remove() is
called before the object is removed from the abo->mem.umap_list. If
aie2_populate_range() runs concurrently, could it find the dying object in
the list and blindly call kref_get(&mapp->refcnt) here after the refcount has
already dropped to zero?

> -	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);

[Severity: High]
This isn't a bug introduced by this patch, but does this code permanently
leak a reference to the mm_struct?

It looks like mmget_not_zero(mm) is called here, but on the successful path
and retry paths later in the function:

    if (ret == -EBUSY) {
        amdxdna_umap_put(mapp);
        goto again;
    }

and:

    mapp->invalid = false;
    up_write(&xdna->notifier_lock);
    amdxdna_umap_put(mapp);
    goto again;

It appears mmput(mm) is missing before jumping back to the again label.

[ ... ]

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 63976c3bcbe0f..20ce304b19ef2 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;

[Severity: High]
This isn't a bug introduced by this patch, but does this invalidate logic
safely handle partial unmaps?

When user space partially unmaps a VMA, mmu_notifier_invalidate_range_start()
notifies the driver with MMU_NOTIFY_UNMAP. However, further down in this
function, hmm_unreg_work is queued unconditionally:

    if (range->event == MMU_NOTIFY_UNMAP) {
        down_write(&xdna->notifier_lock);
        if (!mapp->unmapped) {
            queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
            mapp->unmapped = true;
        }

Since this destroys the notifier for the entire mapping without verifying
if the entire range is being unmapped, could the remainder of the VMA
stay mapped in user space without an interval notifier, leading to device
hardware performing DMA to stale, freed physical pages?

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