Re: [PATCH v2 2/3] drm/xe/oa: Gate OA buffer mmap faults

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/oa: ungated vm_ops->access in xe_oa_vm_ops via xe_bo_vm_access()
- [Medium] drm/xe/oa: race in xe_oa_buffer_fault() allows dummy page install on unplug
--

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

drm/xe/oa: Gate OA buffer mmap faults

This commit adds xe_oa_buffer_fault() and xe_oa_vm_ops to return
VM_FAULT_SIGBUS when faulting on a mapped OA buffer after stream->dead is set,
and rejects new mmaps in xe_oa_mmap() when the stream is dead. It also exposes
xe_bo_cpu_fault() and xe_bo_vm_access() for reuse in xe_oa_vm_ops.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 7e3ebba0300d6..4916101fce541 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -1723,6 +1723,28 @@ static int xe_oa_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +static vm_fault_t xe_oa_buffer_fault(struct vm_fault *vmf)
> +{
> +	struct xe_oa_stream *stream = vmf->vma->vm_file->private_data;
> +
> +	/*
> +	 * Once the stream is dead its OA buffer may already be gone (or about
> +	 * to be). Refuse to (re-)install a PTE rather than letting the
> +	 * generic BO fault handler touch a buffer that's being torn down.
> +	 */
> +	if (READ_ONCE(stream->dead))
> +		return VM_FAULT_SIGBUS;
> +
> +	return xe_bo_cpu_fault(vmf);
> +}

[Severity: Medium]
Can a page fault racing with device unplug, reset, or wedging bypass the
VM_FAULT_SIGBUS gate in xe_oa_buffer_fault() and install dummy zero-page PTEs
across the VMA via xe_bo_cpu_fault()?

In xe_oa_buffer_fault(), READ_ONCE(stream->dead) is checked locklessly outside
the drm_dev_enter() SRCU critical section before calling xe_bo_cpu_fault():

drivers/gpu/drm/xe/xe_bo.c:xe_bo_cpu_fault() {
	...
	if (xe_wedged(xe) || !drm_dev_enter(&xe->drm, &idx))
		return ttm_bo_vm_dummy_page(vmf, vmf->vma->vm_page_prot);
	...
}

If a fault passes the check in xe_oa_buffer_fault() right before
drm_dev_unplug() sets dev->unplugged in xe_device_remove(),
drm_dev_enter(&xe->drm, &idx) in xe_bo_cpu_fault() returns false and calls
ttm_bo_vm_dummy_page() without holding drm_unplug_srcu. Because
synchronize_srcu(&drm_unplug_srcu) does not wait for that fault,
unmap_mapping_range() and OA buffer teardown can finish before
ttm_bo_vm_dummy_page() completes prefaulting zero pages across [vma->vm_start,
vma->vm_end) and returning VM_FAULT_NOPAGE instead of VM_FAULT_SIGBUS.

Similarly, if the device is wedged via xe_wedge_declare() or
prepare_device_for_reset() while stream->dead and dev->unplugged are still
false, a fault on the OA buffer passes xe_oa_buffer_fault() and hits the
xe_wedged(xe) check in xe_bo_cpu_fault(), prefaulting dummy pages instead of
returning VM_FAULT_SIGBUS.

Additionally, in the follow-up commit ("drm/xe/oa: Quiesce OA streams on
driver unbind and PCI reset"), if a fault evaluates READ_ONCE(stream->dead) as
false and is preempted before evaluating
drm_dev_is_unplugged(&stream->oa->xe->drm) while xe_oa_fini() completes
xe_oa_quiesce() and sets oa->xe = NULL, could dereferencing stream->oa->xe
cause a NULL pointer dereference?

> +
> +static const struct vm_operations_struct xe_oa_vm_ops = {
> +	.fault	= xe_oa_buffer_fault,
> +	.open	= ttm_bo_vm_open,
> +	.close	= ttm_bo_vm_close,
> +	.access	= xe_bo_vm_access,
> +};

[Severity: High]
Should xe_oa_vm_ops also gate or omit .access instead of wiring it directly to
xe_bo_vm_access()?

In xe_oa_mmap(), the VMA is configured as read-only with VM_PFNMAP (and
drm_gem_mmap_obj() / ttm_bo_mmap_obj() sets VM_IO | VM_PFNMAP):

drivers/gpu/drm/xe/xe_oa.c:xe_oa_mmap() {
	...
	if (vma->vm_flags & (VM_WRITE | VM_EXEC | VM_SHARED | VM_MAYSHARE)) {
		drm_dbg(&stream->oa->xe->drm, "mmap must be read only\n");
		return -EINVAL;
	}
	vm_flags_mod(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_DONTCOPY,
		     VM_MAYWRITE | VM_MAYEXEC);
	...
}

When userspace accesses this mapping through /proc/<pid>/mem, ptrace(),
process_vm_readv(), or process_vm_writev(), __access_remote_vm() fails in
get_user_page_vma_remote() and falls back to vma->vm_ops->access(), which
calls xe_bo_vm_access():

drivers/gpu/drm/xe/xe_bo.c:xe_bo_vm_access() {
	struct ttm_buffer_object *ttm_bo = vma->vm_private_data;
	struct xe_bo *bo = ttm_to_xe_bo(ttm_bo);
	struct xe_device *xe = xe_bo_device(bo);

	guard(xe_pm_runtime)(xe);
	return ttm_bo_vm_access(vma, addr, buf, len, write);
}

Because vma->vm_private_data retains a GEM reference to the BO until the VMA
is unmapped, calling xe_bo_vm_access() after stream->dead is set bypasses the
stream->dead check, takes a runtime PM reference, and calls ttm_bo_vm_access()
-> xe_ttm_access_memory() on the torn-down OA buffer. When the OA buffer was
placed in VRAM (mert_wa_14026633728) and vram_fini() has cleared
vram->mapping to NULL during unbind, xe_ttm_access_memory() will dereference a
NULL vram->mapping pointer.

Furthermore, because __access_remote_vm() passes write = 1 on FOLL_WRITE and
neither xe_bo_vm_access() nor ttm_bo_vm_access() checks whether write is
non-zero, doesn't wiring .access to xe_bo_vm_access() also allow writing to
the read-only OA buffer via /proc/self/mem or process_vm_writev()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030223056.2266638-5-umesh.nerlige.ramappa@intel.com?part=2