Re: [PATCH v3 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 ed8b71dc76a867bf05d811e39433598fc6bd1502
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/oa: Gate OA buffer mmap faults

This commit exposes xe_bo_cpu_fault() and xe_bo_vm_access() and introduces
xe_oa_buffer_fault() and xe_oa_vm_ops to return VM_FAULT_SIGBUS on OA buffer
page faults once stream->dead is set.

> 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]
As reported by Sashiko on v2 [1], can a page fault on a mapped OA buffer
racing with device unplug, reset, or wedging bypass the VM_FAULT_SIGBUS gate
in xe_oa_buffer_fault() and install dummy pages via xe_bo_cpu_fault()
instead?

In xe_oa_buffer_fault(), READ_ONCE(stream->dead) (and in the follow-up
commit "drm/xe/oa: Quiesce OA streams on driver unbind and PCI reset",
drm_dev_is_unplugged(&stream->oa->xe->drm)) is checked locklessly outside
the drm_dev_enter() SRCU critical section before calling
xe_bo_cpu_fault(vmf):

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() just before
drm_dev_unplug() sets dev->unplugged = true in xe_device_remove(),
drm_dev_enter() in xe_bo_cpu_fault() returns false without holding
drm_unplug_srcu and calls ttm_bo_vm_dummy_page(). Because drm_unplug_srcu is
not held, synchronize_srcu(&drm_unplug_srcu) in drm_dev_unplug() does not
wait for the fault, allowing unmap_mapping_range() in drm_dev_unplug() and
drm_vma_node_unmap() in xe_oa_quiesce_oa_unit() to complete before
ttm_bo_vm_dummy_page() finishes prefaulting dummy zero-page PTEs across
[vma->vm_start, vma->vm_end) and returning VM_FAULT_NOPAGE.

Similarly, when the device is wedged via xe_wedge_declare() or
prepare_device_for_reset() (in xe_pci_error_detected() prior to
xe_pci_error_slot_reset()), xe_wedged(xe) is true while stream->dead and
dev->unplugged are still false, so a fault passes xe_oa_buffer_fault() and
hits ttm_bo_vm_dummy_page() in xe_bo_cpu_fault().

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, dereferencing &stream->oa->xe->drm
will hit 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]
As reported by Sashiko on v2 [2], does wiring xe_oa_vm_ops.access directly
to xe_bo_vm_access() allow userspace to bypass the stream->dead check and
also write to the read-only OA buffer mapping?

Because xe_oa_mmap() sets VM_PFNMAP (and drm_gem_mmap_obj() /
ttm_bo_mmap_obj() sets VM_IO | VM_PFNMAP), accesses through /proc/<pid>/mem,
ptrace(PTRACE_PEEKDATA), process_vm_readv(), or process_vm_writev() fail
get_user_page_vma_remote() in __access_remote_vm() and fall back to
vma->vm_ops->access():

__access_remote_vm()
  -> vma->vm_ops->access()
    -> xe_bo_vm_access()
      -> guard(xe_pm_runtime)(xe)
      -> ttm_bo_vm_access()
        -> ttm_bo_access()
          -> xe_ttm_access_memory()

Because vma->vm_private_data holds a GEM reference to struct xe_bo until
munmap, calling xe_bo_vm_access() after stream->dead is set during PCI
unbind/remove unconditionally takes a runtime PM reference and accesses the
torn-down OA buffer BO. If the OA buffer was allocated in VRAM
(mert_wa_14026633728) and vram_fini() has already cleared
tile->mem.vram->mapping to NULL on unbind, xe_ttm_access_memory() will
dereference a NULL vram->mapping pointer.

Furthermore, although xe_oa_mmap() enforces a read-only mapping by rejecting
VM_WRITE and clearing VM_MAYWRITE, __access_remote_vm() with FOLL_WRITE
invokes vma->vm_ops->access() with write = 1 when get_user_page_vma_remote()
fails, and neither xe_oa_vm_ops nor xe_bo_vm_access() / ttm_bo_vm_access()
rejects write != 0. Should xe_oa_vm_ops wrap or omit .access so dead streams
and write accesses are rejected?

[1] https://lore.kernel.org/r/20261030223056.2266638-7-umesh.nerlige.ramappa@intel.com
[2] https://lore.kernel.org/r/20261030223056.2266638-7-umesh.nerlige.ramappa@intel.com

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