Re: [PATCH v2 4/6] drm/imagination: Add VM_BIND ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Critical] drm/imagination: UAF on page tables in pvr_vm_bind() after
  unlock
--

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

drm/imagination: Add VM_BIND ioctl

This commit adds the DRM_IOCTL_PVR_VM_BIND ioctl to apply a batch of map and
unmap operations to a VM context, and bumps the driver minor version to 2.

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 2d522a8e46ad6..8203d5f081d5d 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1135,6 +1135,168 @@ pvr_ioctl_vm_unmap(struct drm_device *drm_dev, void *raw_args,
[ ... ]
> +static int
> +pvr_ioctl_vm_bind(struct drm_device *drm_dev, void *raw_args,
> +		  struct drm_file *file)
> +{
[ ... ]
> +	descs = pvr_vm_bind_descs_from_uapi(pvr_file, uapi_ops, args->ops.count);
> +	kvfree(uapi_ops);
> +	if (IS_ERR(descs)) {
> +		err = PTR_ERR(descs);
> +		goto err_put_vm_context;
> +	}
> +
> +	err = pvr_vm_bind(vm_ctx, descs, args->ops.count);

[Severity: Critical]
Can concurrent VM operations on the same vm_ctx cause a use-after-free on
op_ctx->curr_page.l0_table and l1_table in pvr_vm_bind()?

drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_bind() {
	...
	mutex_lock(&vm_ctx->lock);

	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);
	...
}

pvr_vm_bind_op_exec() caches raw pointers to L0 and L1 page tables in
op_ctx->curr_page.l0_table and l1_table and defers the final DMA sync until
pvr_vm_bind_ops_free() -> pvr_vm_bind_op_fini() ->
pvr_mmu_op_context_destroy() -> pvr_mmu_op_context_sync(), which runs after
vm_ctx->lock has been dropped:

CPU0                                  CPU1
pvr_ioctl_vm_bind()
  pvr_vm_bind()
    mutex_lock(&vm_ctx->lock)
    pvr_vm_bind_op_exec()
      ...
      op_ctx->curr_page.l0_table = ...
    mutex_unlock(&vm_ctx->lock)
                                      pvr_ioctl_vm_bind() / pvr_ioctl_vm_unmap()
                                        pvr_vm_bind()
                                          mutex_lock(&vm_ctx->lock)
                                          pvr_vm_bind_op_exec()
                                            ...
                                            pvr_page_table_l1_remove()
                                          mutex_unlock(&vm_ctx->lock)
                                          pvr_vm_bind_ops_free()
                                            ...
                                            pvr_page_table_l0_free()
    pvr_vm_bind_ops_free()
      ...
      pvr_mmu_op_context_sync_manual()
        pvr_page_table_l0_sync(op_ctx->curr_page.l0_table)
          <-- use-after-free

Once CPU0 drops vm_ctx->lock, CPU1 can unmap the remaining entries in those
page tables and free them before CPU0 runs pvr_mmu_op_context_sync_manual(),
causing CPU0 to dereference freed page table structs and sync unmapped backing
pages.

A later patch in this series, commit "drm/imagination: Add asynchronous
VM_BIND", calls pvr_mmu_op_context_sync() and pvr_mmu_flush_exec() under
vm_ctx->lock in pvr_vm_bind_run_job(), but synchronous pvr_vm_bind() still
leaves them outside the lock.

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