Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Incompatible VMA flags modification replaces VM_PFNMAP with VM_MIXEDMAP, creating a locally exploitable kernel panic via BUG_ON. - [High] Eager page table population inside the mmap callback bypasses interval tree tracking, risking Use-After-Free. -- --- Patch [1]: [PATCH V1] accel/amxdna: fix page-insertion errors in amdxdna_insert_pages() --- commit c41b85178d2bef0390b7acad007274b457c13742 Author: Lizhi Hou <[email protected]> accel/amxdna: fix page-insertion errors in amdxdna_insert_pages() This commit prevents a reference count underflow when error paths in amdxdna_insert_pages() prematurely close a VMA. It replaces hard error returns with a deferred-fault approach that keeps the VMA alive and retries page insertion later. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index df236a9d8a0a4..493f34473266e 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > @@ -457,8 +474,7 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj > *abo, > &num_pages); [Severity: Critical] This is a pre-existing issue, but does replacing VM_PFNMAP with VM_MIXEDMAP create a locally exploitable kernel panic? Just above this line, vm_flags_mod() sets VM_MIXEDMAP and clears VM_PFNMAP. If an unprivileged userspace process mmaps a non-imported GEM object and then calls madvise(MADV_DONTNEED), the PTEs will be successfully cleared because VM_MIXEDMAP allows this (unlike VM_PFNMAP). When userspace subsequently accesses the memory, drm_gem_shmem_fault() handles the page fault and attempts to map the backing shmem page via vmf_insert_pfn() which calls vmf_insert_pfn_prot(). Because the backing shmem page is normal system memory (pfn_valid(pfn) is true) and the VMA now has VM_MIXEDMAP set, won't this predictably trigger the explicit assertion BUG_ON((vma->vm_flags & VM_MIXEDMAP) && pfn_valid(pfn)), crashing the system? [Severity: High] This is also a pre-existing issue, but does eagerly populating PTEs inside the mmap callback bypass interval tree tracking? At the time the .mmap callback executes, the new VMA is not yet linked into the backing file's interval tree (mapping->i_mmap). The core mm links it via vma_link_file() only after the .mmap callback returns successfully. If a dma-buf exporter concurrently evicts the memory and calls unmap_mapping_range(), won't the invalidation miss this unlinked VMA and fail to shoot down the eagerly populated PTEs, leading to a use-after-free? > if (ret) { > XDNA_ERR(xdna, "Failed insert pages %d", ret); > - vma->vm_ops->close(vma); > - return ret; > + amdxdna_mark_mapp_invalid(abo, vma); > } > > return 0; > @@ -478,9 +494,9 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj > *abo, > fault_ret = handle_mm_fault(vma, vma->vm_start + offset, > FAULT_FLAG_WRITE, NULL); [Severity: High] This isn't a bug introduced by this patch, but does this eager population for dma-buf imports have the same interval tree tracking bypass issue? Since the VMA is not yet linked into the interval tree during the .mmap callback, an invalidation from the exporter might miss these eagerly populated PTEs, resulting in a use-after-free. > if (fault_ret & VM_FAULT_ERROR) { > - vma->vm_ops->close(vma); > XDNA_ERR(xdna, "Fault in page failed"); > - return -EFAULT; > + amdxdna_mark_mapp_invalid(abo, vma); > + break; > } > > offset += PAGE_SIZE; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
