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 <[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. Fixes: e486147c912f ("accel/amdxdna: Add BO import and export") Signed-off-by: Lizhi Hou <[email protected]> > 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/[email protected]?part=1
