Re: [PATCH V2] 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:
- [High] Modifying vma->vm_pgoff in the dma-buf mmap exporter breaks the dma-buf interval tree, preventing memory revocation.
- [High] The patch completely bans mapping the BO at a non-zero offset, which breaks userspace compatibility for partial mappings.
--

commit f5b497b44e071da0d7d3ac56c9b4055ed5127ba8
Author: Lizhi Hou <lizhi.hou@amd.com>
Subject: accel/amdxdna: Fix unsafe use of handle_mm_fault()

This commit replaces the use of handle_mm_fault() from the mmap callback
with a VM_MIXEDMAP and vm_insert_pages() approach for shmem BOs. For imported
BOs, it marks the mapping as invalid and relies on the first command
submission to fault in the pages.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 3f05fa8811af1..612f36a2c0725 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -490,16 +491,12 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,
>  {
>  	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>  	unsigned long num_pages = vma_pages(vma);
> -	unsigned long offset = 0;
>  	int ret;
>  
> -	if (!is_import_bo(abo)) {
> -		ret = drm_gem_shmem_mmap(&abo->base, vma);
> -		if (ret) {
> -			XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
> -			return ret;
> -		}
> -	} else {
> +	if (vma->vm_pgoff - drm_vma_node_start(&to_gobj(abo)->vma_node))
> +		return -EINVAL;

[Severity: High]
Does this check artificially reject valid non-zero offsets that were previously
supported?

Userspace applications that attempt to mmap a sub-region of a Buffer Object
will receive an unexpected -EINVAL error.

If a userspace process calls mmap() on the DRM device fd, passing an offset
that includes both the DRM mmap node offset and a logical offset within the BO,
it seems this strict equality check will fail and ban partial mappings.

> +
> +	if (is_import_bo(abo)) {
>  		vma->vm_private_data = 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;
>  
> -	vma->vm_ops = &drm_gem_shmem_vm_ops;
> +	vma->vm_ops = &amdxdna_gem_mixedmap_vm_ops;
>  	vma->vm_private_data = gobj;
>  
>  	drm_gem_object_get(gobj);
> +	vma->vm_pgoff += drm_vma_node_start(&gobj->vma_node);

[Severity: High]
Could modifying vma->vm_pgoff here corrupt the logical offset used for linking
the VMA into the dma-buf address_space interval tree?

After the callback returns, the core memory management (mmap_region) links the
VMA into the dma-buf's file mapping interval tree using this highly inflated
vm_pgoff.

Because of this, memory revocation mechanisms for dma-bufs might fail to find
and unmap this VMA, leaving stale PTEs that can lead to a use-after-free if
the backing memory is freed.

>  	ret = drm_gem_shmem_mmap(&abo->base, vma);
>  	if (ret)
>  		goto put_obj;

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