Re: [PATCH v2 3/6] drm/imagination: Route map and unmap through pvr_vm_bind()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/imagination: UAF and sync ordering bug in pvr_vm_bind()
--

commit 858616fa9e0f9c862d2be468e58d23d170048080
Author: Gyeyoung Baek <gye976@gmail.com>

drm/imagination: Route map and unmap through pvr_vm_bind()

This commit consolidates map and unmap paths into a single pvr_vm_bind()
entry point taking an array of struct pvr_vm_bind_desc.

> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 12ebf0daf88db..2f959cd5e9047 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c

[ ... ]

> @@ -683,188 +692,191 @@ void pvr_destroy_vm_contexts_for_file(struct pvr_file *pvr_file)

[ ... ]

> -static int
> -pvr_vm_unmap_obj_locked(struct pvr_vm_context *vm_ctx,
> -			struct pvr_gem_object *pvr_obj,
> -			u64 device_addr, u64 size)
> +static struct pvr_vm_bind_op *
> +pvr_vm_bind_ops_create(struct pvr_vm_context *vm_ctx,
> +		       const struct pvr_vm_bind_desc *descs, u32 desc_count)
>  {
> -	struct pvr_vm_bind_op bind_op = {0};
> -
> -	int err = pvr_vm_bind_op_unmap_init(&bind_op, vm_ctx, pvr_obj,
> -					    device_addr, size);
> -	if (err)
> -		return err;
> +	struct pvr_vm_bind_op *bind_ops;
> +	int err;
>  
> -	pvr_gem_object_get(pvr_obj);
> +	bind_ops = kvzalloc_objs(*bind_ops, desc_count, GFP_KERNEL_ACCOUNT);
> +	if (!bind_ops)
> +		return ERR_PTR(-ENOMEM);
>  
> -	err = pvr_vm_bind_op_exec(&bind_op);
> +	for (u32 prepared = 0; prepared < desc_count; prepared++) {
> +		err = pvr_vm_bind_op_init(&bind_ops[prepared], vm_ctx,
> +					  &descs[prepared]);
> +		if (err) {
> +			pvr_vm_bind_ops_free(bind_ops, prepared);
> +			/* Release the vm_bos that fini() deferred. */
> +			drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr);
> +			return ERR_PTR(err);
> +		}
>  
> -	pvr_vm_bind_op_fini(&bind_op);
> +		cond_resched();
> +	}
>  
> -	return err;
> +	return bind_ops;
>  }
>  
>  /**
> - * pvr_vm_unmap_obj() - Unmap an already mapped section of device-virtual
> - * memory.
> + * pvr_vm_bind() - Apply a batch of bind operations to a VM context.
>   * @vm_ctx: Target VM context.
> - * @pvr_obj: Target PowerVR memory object.
> - * @device_addr: Virtual device address at the start of the target mapping.
> - * @size: Size of the target mapping.
> + * @descs: Operations to apply, in array order.
> + * @desc_count: Number of entries in @descs.
> + *
> + * Not atomic: if an operation fails, the earlier ones stay applied.
>   *
>   * Return:
> - *  * 0 on success,
> - *  * Any error encountered by pvr_vm_unmap_obj_locked.
> + *  * 0 on success, or
> + *  * A negative error code on failure.
>   */
> -int
> -pvr_vm_unmap_obj(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
> -		 u64 device_addr, u64 size)
> +int pvr_vm_bind(struct pvr_vm_context *vm_ctx,
> +		const struct pvr_vm_bind_desc *descs, u32 desc_count)
>  {
> -	int err;
> +	struct pvr_vm_bind_op *bind_ops;
> +	int err = 0;
> +
> +	bind_ops = pvr_vm_bind_ops_create(vm_ctx, descs, desc_count);
> +	if (IS_ERR(bind_ops))
> +		return PTR_ERR(bind_ops);
>  
>  	mutex_lock(&vm_ctx->lock);
> -	err = pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj, device_addr, size);
> +
> +	for (u32 i = 0; !err && i < desc_count; i++)
> +		err = pvr_vm_bind_op_exec(&bind_ops[i]);
> +
>  	mutex_unlock(&vm_ctx->lock);
>  
> +	pvr_vm_bind_ops_free(bind_ops, desc_count);
>  	drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr);
>  
>  	return err;
>  }

[ ... ]

> @@ -862,25 +862,21 @@ pvr_vm_unmap(struct pvr_vm_context *vm_ctx, u64 device_addr, u64 size)
>  void
>  pvr_vm_unmap_all(struct pvr_vm_context *vm_ctx)
>  {
> -	u64 addr = vm_ctx->gpuvm_mgr.mm_start;
> -	u64 end = vm_ctx->gpuvm_mgr.mm_start + vm_ctx->gpuvm_mgr.mm_range;
> -	u64 va_addr, va_range;
> -
> -	mutex_lock(&vm_ctx->lock);
> -
> -	for (; addr < end; addr = va_addr + va_range) {
> -		struct pvr_gem_object *pvr_obj;
> -		struct drm_gpuva *va;
> -
> -		va = drm_gpuva_find_first(&vm_ctx->gpuvm_mgr, addr, end - addr);
> -		if (!va)
> -			break;
> -
> -		pvr_obj = gem_to_pvr_gem(va->gem.obj);
> -		va_addr = va->va.addr;
> -		va_range = va->va.range;
> +	const struct pvr_vm_bind_desc desc = {
> +		.type = PVR_VM_BIND_TYPE_UNMAP,
> +		.device_addr = vm_ctx->gpuvm_mgr.mm_start,
> +		.size = vm_ctx->gpuvm_mgr.mm_range,
> +	};
> +	struct pvr_vm_bind_op bind_op = {0};
>  
> -		WARN_ON(pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj, va_addr, va_range));
> -	}
> +	if (WARN_ON(pvr_vm_bind_op_unmap_init(&bind_op, vm_ctx, &desc)))
> +		return;
>  
> +	mutex_lock(&vm_ctx->lock);
> +	WARN_ON(pvr_vm_bind_op_exec(&bind_op));
>  	mutex_unlock(&vm_ctx->lock);
>  
> +	pvr_vm_bind_op_fini(&bind_op);
>  	drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr);
>  }

[Severity: High]
Can calling pvr_vm_bind_ops_free() and pvr_vm_bind_op_fini() after dropping
vm_ctx->lock cause a use-after-free on op_ctx->curr_page.l0_table and
l1_table?

During pvr_vm_bind_op_exec(), pvr_mmu_map() and pvr_mmu_unmap() leave raw
pointers to the active page tables in op_ctx->curr_page.l0_table and
l1_table with pending syncs recorded in op_ctx->sync_level_required.
Previously, pvr_vm_unmap_obj_locked() ran pvr_vm_bind_op_fini() ->
pvr_mmu_op_context_destroy() -> pvr_mmu_op_context_sync() while holding
vm_ctx->lock.  With vm_ctx->lock dropped before pvr_vm_bind_ops_free(), a
second thread can unmap the remaining entries in the same L0 or L1 table
and free it before the first thread syncs:

CPU0                                  CPU1
pvr_vm_bind()
  mutex_lock(&vm_ctx->lock)
  pvr_vm_bind_op_exec(&bind_ops[0])
    /* leaves curr_page.l0_table = T0,
       T0->entry_count == 1 */
  mutex_unlock(&vm_ctx->lock)
                                      pvr_vm_bind()
                                        mutex_lock(&vm_ctx->lock)
                                        pvr_vm_bind_op_exec(...)
                                          /* unmaps last entry in T0,
                                             queues T0 on l0_free_tables */
                                        mutex_unlock(&vm_ctx->lock)
                                        pvr_vm_bind_ops_free()
                                          ...
                                            pvr_page_table_l0_free(T0)
  pvr_vm_bind_ops_free()
    pvr_vm_bind_op_fini(&bind_ops[0])
      pvr_mmu_op_context_destroy()
        ...
          pvr_page_table_l0_sync(T0)  <-- use-after-free

In addition, when desc_count > 1 in pvr_vm_bind(), deferring
pvr_mmu_op_context_sync() until pvr_vm_bind_ops_free() runs after the loop
breaks the leaf-to-root (L0 -> L1 -> L2) DMA sync order across operations in
the same batch (and a later unmap in the batch can free a table still
referenced by an earlier op's curr_page).  For instance, if bind_ops[0]
allocates an L1 table T1 (requiring a level 2 sync) and bind_ops[1] attaches
a new L0 table T0_b to T1, freeing bind_ops[0] first syncs T1 and L2 before
freeing bind_ops[1] syncs T0_b.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-pvr-vm-bind-v2-0-730103d2cc9a@gmail.com?part=3