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 <[email protected]> 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/[email protected]?part=1
