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

Lizhi Hou posted 1 patch 2 weeks, 1 day ago
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(-)
[PATCH V3] accel/amdxdna: Fix VMA access race
Posted by Lizhi Hou 2 weeks, 1 day ago
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
Posted by Mario Limonciello 2 weeks, 1 day ago

On 6/8/26 20:12, Lizhi Hou wrote:
> 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`

I'm thinking we should make sure that the other existing bugs sashiko 
raised are fixed first
> 
>   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;
Re: [PATCH V3] accel/amdxdna: Fix VMA access race
Posted by Lizhi Hou 2 weeks ago
On 6/9/26 15:46, Mario Limonciello wrote:
>
>
> On 6/8/26 20:12, Lizhi Hou wrote:
>> 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`
>
> I'm thinking we should make sure that the other existing bugs sashiko 
> raised are fixed first

I am working on other fixes as well. And they are not related to this 
issue.

This is an use-after-free and can potentially crash the system in some 
cases. Fixing it early may also avoid receiving more AI scan reports on 
this. :)


Thanks,

Lizhi

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