Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Taking `vm_ctx->lock` inside a dma-fence signalling critical section creates a deadlock risk because legacy ioctl paths hold this lock during `GFP_KERNEL` memory allocations. - [High] The `DRM_IOCTL_PVR_VM_BIND` ioctl accepts an unbounded number of operations, allowing a local denial-of-service via unbounded kernel memory allocations. --
--- Patch [3]: [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl --- commit d69b2e6682fdd9c9fc81f9944e1b72971b7b0c0f Author: Gyeyoung Baek <[email protected]> Subject: drm/imagination: Add async VM_BIND ioctl This commit adds a DRM_IOCTL_PVR_VM_BIND ioctl to the PowerVR driver to support Vulkan sparse binding needs. It allows batching bind and sync operations into a single request. Requests can be run asynchronously as drm_sched jobs when the DRM_PVR_VM_BIND_ASYNC flag is provided. > diff --git a/drivers/gpu/drm/imagination/pvr_drv.c > b/drivers/gpu/drm/imagination/pvr_drv.c > index 5c965ef0274ff..ec6ed610e29e1 100644 > --- a/drivers/gpu/drm/imagination/pvr_drv.c > +++ b/drivers/gpu/drm/imagination/pvr_drv.c [ ... ] > @@ -1130,6 +1130,93 @@ 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) > +{ > + struct drm_pvr_ioctl_vm_bind_args *args = raw_args; [ ... ] > + vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle); > + if (!vm_ctx) { > + err = -EINVAL; > + goto err_drm_dev_exit; > + } > + [Severity: High] Could args->ops.count or args->sync_ops.count be arbitrarily large here? Without an upper bound, could an unprivileged user exhaust system memory by passing an extremely large count, as these allocations bypass memory cgroup limits? > + if (args->ops.count) { > + err = PVR_UOBJ_GET_ARRAY(uapi_ops, &args->ops); > + if (err) > + goto err_put_vm_context; > + } > + > + if (args->sync_ops.count) { > + err = PVR_UOBJ_GET_ARRAY(sync_ops, &args->sync_ops); > + if (err) > + goto err_free_uapi_ops; > + } > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c > b/drivers/gpu/drm/imagination/pvr_vm.c > index 45df76e61f7cf..f92bfeacd7ac4 100644 > --- a/drivers/gpu/drm/imagination/pvr_vm.c > +++ b/drivers/gpu/drm/imagination/pvr_vm.c [ ... ] > @@ -1174,3 +1242,439 @@ pvr_vm_get_fw_mem_context(struct pvr_vm_context > *vm_ctx) [ ... ] > +static struct dma_fence * > +pvr_vm_bind_run_job(struct drm_sched_job *sched_job) > +{ > + struct pvr_vm_bind_job *job = to_pvr_vm_bind_job(sched_job); > + struct pvr_vm_context *vm_ctx = job->vm_ctx; > + int err = 0; > + bool cookie; > + > + if (pvr_vm_context_is_unusable(vm_ctx)) > + return ERR_PTR(-ECANCELED); > + > + cookie = dma_fence_begin_signalling(); > + [Severity: High] Does acquiring vm_ctx->lock inside the dma-fence signalling critical section create a circular deadlock risk? The legacy pvr_vm_unmap() path acquires vm_ctx->lock to look up the mapping, and then calls pvr_vm_unmap_obj_locked() -> pvr_vm_bind_op_unmap_init(). That init function performs GFP_KERNEL allocations under this same lock: bind_op->prev_va = kzalloc_obj(*bind_op->prev_va); bind_op->next_va = kzalloc_obj(*bind_op->next_va); ... bind_op->mmu_op_ctx = pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0); Direct memory reclaim during these allocations might wait on dma-fences to be signalled, which could deadlock if this lock is held. > + mutex_lock(&vm_ctx->lock); > + > + for (u32 i = 0; i < job->op_count; i++) { > + err = pvr_vm_bind_op_exec(&job->ops[i]); > + if (err) > + break; > + } [ ... ] > +static struct pvr_vm_bind_op * > +pvr_vm_bind_ops_create_from_uapi(struct pvr_vm_context *vm_ctx, > + struct pvr_file *pvr_file, > + const struct drm_pvr_vm_bind_op *uapi_ops, > + u32 op_count) > +{ > + struct pvr_vm_bind_op *ops; > + int err; > + [Severity: High] Could op_count be unbounded here? Since this allocates using GFP_KERNEL without __GFP_ACCOUNT, large allocations won't be charged to a memcg, potentially leading to system memory exhaustion or soft lockups during the subsequent loop. > + ops = kvzalloc_objs(*ops, op_count, GFP_KERNEL); > + if (!ops) > + return ERR_PTR(-ENOMEM); > + > + for (u32 prepared = 0; prepared < op_count; prepared++) { > + err = pvr_vm_bind_op_init_from_uapi(&ops[prepared], vm_ctx, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
