Re: [PATCH v3 1/5] drm/xe/oa: Use the drm device address_space for the OA stream fd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe/oa: missing vm_pgoff check in xe_oa_mmap() allows OOB fault
--

commit fbfcd89abe0e799c3080815623130528c7cfb1de
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/oa: Use the drm device address_space for the OA stream fd

This commit points the OA stream file's f_mapping at the DRM device's
anon_inode address_space so that drm_dev_unplug() zaps OA buffer mmaps
alongside GEM mmaps when the device is unplugged.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 838802482adaf..09043bc90de94 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -1869,6 +1869,7 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
>  {
>  	struct xe_oa_stream *stream;
>  	struct drm_syncobj *ufence_syncobj;
> +	struct file *file;
>  	int stream_fd;
>  	int ret;
>  
> @@ -1910,12 +1911,27 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
>  			goto err_destroy;
>  	}
>  
> -	stream_fd = anon_inode_getfd("[xe_oa]", &xe_oa_fops, stream, 0);
> +	stream_fd = get_unused_fd_flags(0);
>  	if (stream_fd < 0) {
>  		ret = stream_fd;
>  		goto err_disable;
>  	}
>  
> +	file = anon_inode_getfile("[xe_oa]", &xe_oa_fops, stream, 0);
> +	if (IS_ERR(file)) {
> +		ret = PTR_ERR(file);
> +		put_unused_fd(stream_fd);
> +		goto err_disable;
> +	}
> +
> +	/*
> +	 * Don't use the address_space of the anon inode singleton, which is shared with
> +	 * unrelated anon files. Use the drm device's address_space instead, same as drm fds,
> +	 * so that drm_dev_unplug() also zaps the OA buffer mmaps.
> +	 */
> +	file->f_mapping = oa->xe->drm.anon_inode->i_mapping;
> +	fd_install(stream_fd, file);

[Severity: High]
Does sharing oa->xe->drm.anon_inode->i_mapping require xe_oa_mmap() to
reject a non-zero vma->vm_pgoff?

The commit message notes that GEM mmaps live at fake offsets starting at
DRM_FILE_PAGE_OFFSET_START while userspace maps the OA buffer at offset 0.
However, in drivers/gpu/drm/xe/xe_oa.c, xe_oa_mmap() only checks the VMA
size and does not verify that vma->vm_pgoff is 0:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_mmap() {
	...
	/* Can mmap the entire OA buffer or nothing (no partial OA buffer mmaps) */
	if (vma->vm_end - vma->vm_start != xe_bo_size(bo)) {
		drm_dbg(&stream->oa->xe->drm, "Wrong mmap size, must be OA buffer size\n");
		return -EINVAL;
	}

	return drm_gem_mmap_obj(&bo->ttm.base, xe_bo_size(bo), vma);
}

Because stream_fd is backed by anon_inode_inode (which is not S_ISREG),
do_mmap() allows arbitrary page-aligned offsets, and neither xe_oa_mmap()
nor the drm_gem_mmap_obj() -> xe_gem_object_mmap() -> drm_gem_ttm_mmap() ->
ttm_bo_mmap_obj() call chain checks vma->vm_pgoff.

If userspace calls mmap() on stream_fd with a non-zero offset in the
DRM_FILE_PAGE_OFFSET_START range, the OA VMA is inserted into
oa->xe->drm.anon_inode->i_mapping->i_mmap at [vma->vm_pgoff,
vma->vm_pgoff + vma_pages(vma) - 1], colliding with GEM BO vma_node ranges
so that drm_vma_node_unmap() on a GEM BO at that range will unexpectedly
zap the OA VMA.

In addition, stream->oa_buffer.bo is allocated in xe_oa_alloc_oa_buffer()
as ttm_bo_type_kernel, so drm_vma_node_start(&bo->ttm.base.vma_node) is 0.
When a page fault occurs via xe_bo_cpu_fault() -> __xe_bo_cpu_fault() ->
ttm_bo_vm_fault_reserved() in drivers/gpu/drm/ttm/ttm_bo_vm.c:

drivers/gpu/drm/ttm/ttm_bo_vm.c:ttm_bo_vm_fault_reserved() {
	...
	page_offset = linear_page_index(vma, address) -
		drm_vma_node_start(&bo->base.vma_node);
	page_last = vma_end_pgoff(vma) -
		drm_vma_node_start(&bo->base.vma_node);

	if (unlikely(page_offset >= PFN_UP(bo->base.size)))
		return VM_FAULT_SIGBUS;
	...
}

If 0 < vma->vm_pgoff < PFN_UP(bo->base.size) and userspace faults at an
address where page_offset < PFN_UP(bo->base.size) (for example,
PFN_UP(bo->base.size) - 1), the initial bounds check passes, and the
prefault loop increments page_offset up to page_last - 1 (which is
vma->vm_pgoff + PFN_UP(bo->base.size) - 1), reading past the end of
ttm->pages[] or the BO's VRAM resource and mapping out-of-bounds physical
pages into userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104005221.3230003-7-umesh.nerlige.ramappa@intel.com?part=1