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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use-after-free on `mapp->vma` during MMU notifier invalidation.
- [High] Memory flag `AS_UNEVICTABLE` is permanently leaked on mmap error paths.

Pre-existing issues:
- [High] Pre-existing use-after-free on `mapp->vma` in debug logging.
- [High] Incorrect management of `AS_UNEVICTABLE` on shared file mappings and swapped `vma->vm_file`.
--

commit 7b0235d032796ef659fe1d987ecb0649b81f544e
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.

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

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 63976c3bcbe0f..80dd183ecba92 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -271,6 +271,10 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni,
>  	if (range->event == MMU_NOTIFY_UNMAP) {
>  		down_write(&xdna->notifier_lock);
>  		if (!mapp->unmapped) {
> +			if (is_import_bo(abo) && mapp->vma->vm_file &&
> +			    mapp->vma->vm_file->f_mapping)
> +				mapping_clear_unevictable(mapp->vma->vm_file->f_mapping);

[Severity: High]
Does this code still have a use-after-free on mapp->vma?

The mmu_interval_notifier tracks virtual address ranges, not VMA lifetimes.
If a VMA is split, merged, or partially unmapped, the original
vm_area_struct can be freed without immediately invalidating the interval
notifier. When a subsequent munmap triggers MMU_NOTIFY_UNMAP, mapp->vma
could be a dangling pointer.


[Severity: High]
Can this code permanently leak the AS_UNEVICTABLE flag on mmap error paths?

If amdxdna_insert_pages() fails during mmap, the error path calls
amdxdna_hmm_unregister(), which sets mapp->unmapped = true but does not
clear the flag. Consequently, the flag might never be cleared because this
invalidation handler skips the clearing when unmapped is true.


[Severity: High]
This isn't a bug introduced by this patch, but does this clear the wrong
file's mapping?

For imported dma-buf BOs, amdxdna_hmm_register() sets AS_UNEVICTABLE on
vma->vm_file->f_mapping. At this time, vma->vm_file is the XDNA device file.

Subsequently, amdxdna_insert_pages() calls dma_buf_mmap(), which swaps
vma->vm_file to the dma-buf file.

When the VMA is later unmapped, mapping_clear_unevictable() is called on the
new vma->vm_file (the dma-buf). This leaves the XDNA device file permanently
locked as unevictable, while the dma-buf file incorrectly loses its
unevictable status for all other clients.


[Severity: High]
This is a pre-existing issue, but does amdxdna_hmm_invalidate() have another
potential use-after-free on the VMA pointer earlier in the function?

Prior to this patch, amdxdna_hmm_invalidate() accesses mapp->vma->vm_start
and mapp->vma->vm_end for an XDNA_DBG log statement:

    XDNA_DBG(xdna, "Invalidating range 0x%lx, 0x%lx, type %d",
             mapp->vma->vm_start, mapp->vma->vm_end, abo->type);

Because VMAs can be freed by mprotect() or vma_merge() without triggering an
MMU notifier, mapp->vma can be a dangling pointer when an invalidation event
later occurs.

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

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