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

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

[Changyul Lee]

vm_bind_ioctl_ops_create() stores SVM ranges, each with a reference, in
op->prefetch_range.range for every PREFETCH op on a CPU address mirror
VMA. xe_vma_ops_fini() releases them only for the ops on vops->list.
When vm_bind_ioctl_ops_parse() fails, the ops after the failing one are
never added to that list, and xe_vm_bind_ioctl() frees them with
drm_gpuva_ops_free(), leaking their XArray nodes and range references.
A PREFETCH that covers a userptr VMA and a CPU address mirror VMA after
it does this when pinning the userptr pages fails.

Release the prefetch ranges of all ops before freeing them. Ranges
already released through vops->list leave an empty XArray, which
xe_vma_svm_prefetch_ranges_fini() skips.

Fixes: 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM ranges")
Signed-off-by: Changyul Lee <lcy8047@gmail.com>
---
Found with a static analysis tool we are developing.

Compile-tested only, with x86_64_defconfig plus DRM_XE, DRM_XE_GPUSVM
and W=1.

 drivers/gpu/drm/xe/xe_vm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index d25de5652767..ee35ead79371 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);

base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.43.0

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

[sashiko-bot]

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