Re: [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Flawed VMA offset calculation in amdxdna_gem_mixedmap_fault causes data corruption on VMA splits and DMA-buf mmaps.

Pre-existing issues:
- [High] DMA-buf mmaps bypass HMM notifier registration, causing missing pre-faulting and unsafe page table teardown.
--

commit 5de79a60bd0ef403c4515e6d277ad3386e515ac9
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix unsafe use of handle_mm_fault()

This commit removes handle_mm_fault() from the mmap callback because the
VMA is not yet linked. For imported BOs, it marks the mapping invalid and
relies on the first command submission to fault in the pages. For shmem
BOs, it sets VM_MIXEDMAP and implements amdxdna_gem_mixed_vm_ops to handle
the page faults.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 3f05fa8811af1..f07f63a194537 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> +static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct drm_gem_object *gobj = vma->vm_private_data;
> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
> +	loff_t num_pages = gobj->size >> PAGE_SHIFT;
> +	vm_fault_t ret = VM_FAULT_SIGBUS;
> +	pgoff_t page_offset;
> +	struct page *page;
> +
> +	/*
> +	 * Partial free of vma is unexpected. Otherwise, the wrong page
> +	 * will be faulted in and the user application may crash itself.
> +	 */
> +	page_offset = vmf->pgoff - vma->vm_pgoff;

[Severity: High]
Will this calculation cause the wrong pages to be mapped on VMA splits or
non-zero offset DMA-buf mmaps?

Because the core MM computes vmf->pgoff as:
vmf->pgoff = vma->vm_pgoff + ((vmf->address - vma->vm_start) >> PAGE_SHIFT)

The subtraction here cancels out the VMA's logical offset, causing it to
always yield the offset relative to the current VMA start. If a user mmaps a
GEM object, unmaps the first half to split the VMA, and then triggers a page
fault on the second half (e.g., after MADV_DONTNEED), could this erroneously
map pages[0] into the middle of the object?

> +
> +	dma_resv_lock(gobj->resv, NULL);

[ ... ]

>  static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  {
>  	struct drm_gem_object *gobj = dma_buf->priv;
>  	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
> -	unsigned long num_pages = vma_pages(vma);
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does amdxdna_gem_dmabuf_mmap() need to
call amdxdna_hmm_register() here?

Without this call, the VMA is not added to the HMM tracking list. If a user
exports a BO to a DMA-buf, mmaps the DMA-buf, and passes the virtual address
to the NPU, the driver might fail to pre-fault the pages before execution.

Additionally, if the user unmaps the VMA while the NPU is active, could the
lack of an MMU interval notifier bypass the driver's teardown stall
(dma_resv_wait_timeout()), allowing the CPU to tear down page tables while
the NPU is still executing and causing IOMMU translation faults?

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