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

[sashiko-bot@kernel.org]

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