Re: [PATCH] drm/xe: Release the SVM prefetch ranges of unparsed ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/xe: use-after-free of xe_vma in
  xe_vma_svm_prefetch_op_fini()
--

commit 8a35eb7044c3a45a355bc03d6a440aba0d3a1369
Author: Changyul Lee <lcy8047@gmail.com>

drm/xe: Release the SVM prefetch ranges of unparsed ops

This commit calls xe_svm_prefetch_gpuva_ops_fini() on each op list before
freeing it in xe_vm_bind_ioctl() so that SVM prefetch ranges allocated by
vm_bind_ioctl_ops_create() are released even for ops that were not parsed
onto vops.list.

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index d25de5652767b..ee35ead79371e 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -4208,8 +4208,10 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  		vm_bind_ioctl_ops_unwind(vm, ops, args->num_binds);
>  	xe_vma_ops_fini(&vops);
>  	for (i = args->num_binds - 1; i >= 0; --i)
> -		if (ops[i])
> +		if (ops[i]) {
> +			xe_svm_prefetch_gpuva_ops_fini(ops[i]);
>  			drm_gpuva_ops_free(&vm->gpuvm, ops[i]);
> +		}
>  free_syncs:
>  	if (err == -ENODATA)
>  		err = vm_bind_ioctl_signal_fences(vm, q, syncs, num_syncs);

[Severity: High]
Can calling xe_svm_prefetch_gpuva_ops_fini() after
vm_bind_ioctl_ops_unwind() and vm_bind_ioctl_ops_execute() cause a
use-after-free of struct xe_vma in xe_vma_svm_prefetch_op_fini()?

drivers/gpu/drm/xe/xe_vm.c:xe_vma_svm_prefetch_op_fini() {
	struct xe_vma *vma;

	vma = gpuva_to_vma(op->base.prefetch.va);

	if (op->base.op == DRM_GPUVA_OP_PREFETCH && xe_vma_is_cpu_addr_mirror(vma))
		xe_vma_svm_prefetch_ranges_fini(op);
}

In a multi-op bind ioctl, if bind_ops[0] maps a new VMA and commits it
into vm->gpuvm, a subsequent PREFETCH in bind_ops[1] over that range
stores &vma->gpuva in op->base.prefetch.va without taking a VMA reference.
If parsing bind_ops[1] fails (for example when pinning userptr pages
fails), vm_bind_ioctl_ops_unwind() runs first and frees bind_ops[0]'s VMA
synchronously via xe_vma_op_unwind() -> xe_vma_destroy_unlocked().
Similarly, if bind_ops[0] prefetches an existing VMA and bind_ops[1]
unmaps or remaps it, vm_bind_ioctl_ops_execute() ->
vm_bind_ioctl_ops_fini() destroys the VMA via xe_vma_destroy() before
reaching unwind_ops.

When xe_vma_ops_fini() and the unconditional
xe_svm_prefetch_gpuva_ops_fini() then run, xe_vma_is_cpu_addr_mirror(vma)
reads vma->gpuva.flags from the freed VMA.  If XE_VMA_SYSTEM_ALLOCATOR is
set in the freed or reused memory on a non-SVM prefetch op,
op->prefetch.region overlaps xa_lock in the struct xe_vma_op union and
xe_vma_svm_prefetch_ranges_fini() passes a corrupted XArray to
xa_destroy(); if the bit is cleared on a freed SVM VMA, the XArray nodes
and range references are leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113044239.2185984-1-lcy8047@gmail.com?part=1