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

[sashiko-bot@kernel.org]

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