Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Exposing a partially initialized context via `xa_alloc` allows concurrent destruction, leading to a NULL pointer dereference or Use-After-Free. - [Critical] Concurrent execution of `pvr_submit_jobs` and `pvr_context_destroy` leads to leaked jobs, contexts, and Use-After-Free vulnerabilities. - [High] Use-After-Free of `ctx->fw_obj` in the `pvr_context_create` error path when `xa_alloc` fails. - [High] ABBA deadlock between pvr_queue_device_pre_reset() and the DRM scheduler run job worker. -- commit 98944fa2f1c44497ea7e4317dca4599e33e19c35 Author: Brajesh Gupta <[email protected]> drm/imagination: Fix double call to drm_sched_entity_fini() This commit prevents a double call to drm_sched_entity_fini() during context destruction by only calling it in the pvr_context_create() failure path. The prior logic incorrectly triggered entity destruction from both pvr_queue_kill() and pvr_queue_destroy(). > diff --git a/drivers/gpu/drm/imagination/pvr_context.c > b/drivers/gpu/drm/imagination/pvr_context.c > index eba4694400b5a..b6f9e078315dc 100644 > --- a/drivers/gpu/drm/imagination/pvr_context.c > +++ b/drivers/gpu/drm/imagination/pvr_context.c [ ... ] > @@ -349,7 +351,7 @@ int pvr_context_create(struct pvr_file *pvr_file, struct > drm_pvr_ioctl_create_co > pvr_fw_object_destroy(ctx->fw_obj); > > err_destroy_queues: > - pvr_context_destroy_queues(ctx); > + pvr_context_destroy_queues(ctx, true); [Severity: High] This is a pre-existing issue, but does this error path lead to a use-after-free if context handles or memory are exhausted? If xa_alloc() fails earlier in pvr_context_create(), the error path jumps to err_destroy_fw_obj, which calls pvr_fw_object_destroy(ctx->fw_obj). This frees the firmware object but leaves the pointer in ctx->fw_obj intact. Execution then falls through to err_destroy_queues, which calls pvr_context_destroy_queues() -> pvr_queue_destroy() -> pvr_queue_cleanup_fw_context(). Since queue->ctx->fw_obj is non-NULL, pvr_queue_cleanup_fw_context() will call pvr_fw_structure_cleanup() on the already-freed pointer, causing a use-after-free: drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_cleanup_fw_context() { if (!queue->ctx->fw_obj) return 0; return pvr_fw_structure_cleanup(queue->ctx->pvr_dev, ...); } Could ctx->fw_obj be set to NULL after destruction, or could this cleanup logic be restructured to avoid accessing the freed pointer? [Severity: Critical] This is also a pre-existing issue, but does the successful creation path expose a partially initialized context to userspace? In pvr_context_create(), after successfully allocating a handle via xa_alloc(), the context handle is immediately visible to userspace: drivers/gpu/drm/imagination/pvr_context.c:pvr_context_create() { err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL); if (err) goto err_destroy_fw_obj; spin_lock(&pvr_dev->ctx_list_lock); list_add_tail(&ctx->file_link, &pvr_file->contexts); ... } If a concurrent thread guesses the newly allocated handle and calls pvr_ioctl_destroy_context() before list_add_tail() completes, it will find the context, remove it, and drop its reference. This triggers pvr_context_release(), which calls list_del(&ctx->file_link). Since ctx->file_link is still NULL, this dereferences a NULL pointer. If list debugging catches it, the original thread might still access the freed context to add it to the list, leading to a use-after-free. Should the context be fully initialized and added to the list before it is exposed via xa_alloc()? > > err_free_ctx_id: > /* [ ... ] > @@ -384,7 +386,7 @@ pvr_context_release(struct kref *ref_count) > spin_unlock(&pvr_dev->ctx_list_lock); > > xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id); > - pvr_context_destroy_queues(ctx); > + pvr_context_destroy_queues(ctx, false); > pvr_fw_object_destroy(ctx->fw_obj); > kfree(ctx->data); > pvr_vm_context_put(ctx->vm_ctx); > diff --git a/drivers/gpu/drm/imagination/pvr_queue.c > b/drivers/gpu/drm/imagination/pvr_queue.c > index 7ed60e1c1a866..941c017399fc3 100644 > --- a/drivers/gpu/drm/imagination/pvr_queue.c > +++ b/drivers/gpu/drm/imagination/pvr_queue.c [ ... ] > @@ -1439,11 +1439,12 @@ void pvr_queue_kill(struct pvr_queue *queue) > /** > * pvr_queue_destroy() - Destroy a queue. > * @queue: The queue to destroy. > + * @cleanup_queue_entity: Whether to cleanup the queue entity. > * > * Cleanup the queue and free the resources attached to it. Should be > * called from the context release function. > */ > -void pvr_queue_destroy(struct pvr_queue *queue) > +void pvr_queue_destroy(struct pvr_queue *queue, bool cleanup_queue_entity) > { > if (!queue) > return; > @@ -1453,7 +1454,8 @@ void pvr_queue_destroy(struct pvr_queue *queue) > mutex_unlock(&queue->ctx->pvr_dev->queues.lock); [Severity: High] This is a pre-existing issue, but does taking pvr_dev->queues.lock here or during GPU reset risk a deadlock with the scheduler's run job worker? During a GPU reset, pvr_queue_device_pre_reset() takes queues.lock and then calls pvr_queue_stop() -> drm_sched_stop() -> drm_sched_wqueue_stop(), which waits for the currently executing scheduler job to finish: drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_device_pre_reset() { mutex_lock(&pvr_dev->queues.lock); list_for_each_entry(queue, &pvr_dev->queues.idle, node) pvr_queue_stop(queue, NULL); ... } Concurrently, if the scheduler is executing drm_sched_run_job_work() -> pvr_queue_run_job() -> pvr_queue_submit_job_to_cccb() -> pvr_queue_update_active_state(), it will try to acquire the same queues.lock: drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_update_active_state() { mutex_lock(&pvr_dev->queues.lock); pvr_queue_update_active_state_locked(queue); mutex_unlock(&pvr_dev->queues.lock); } This creates a scenario where the reset path holds queues.lock and waits for the job worker to finish, while the job worker is blocked trying to acquire queues.lock. Can this deadlock be avoided? > > drm_sched_fini(&queue->scheduler); > - drm_sched_entity_fini(&queue->entity); > + if (cleanup_queue_entity) > + drm_sched_entity_fini(&queue->entity); [Severity: Critical] This isn't a bug introduced by this patch, but does killing the queue entity early lead to leaked jobs and a use-after-free if jobs are still being pushed? In pvr_context_destroy(), if another thread is simultaneously executing pvr_submit_jobs(), it might hold a valid reference to the context. However, pvr_context_destroy() calls pvr_context_kill_queues(), which forcibly destroys the scheduler entity before the context is actually released: drivers/gpu/drm/imagination/pvr_context.c:pvr_context_destroy() { pvr_context_kill_queues(ctx); /* Release the reference held by the handle set. */ pvr_context_put(ctx); } This destroys the DRM scheduler entity via drm_sched_entity_destroy(), which also frees entity->stats. Meanwhile, the concurrent thread in pvr_submit_jobs() can proceed to push jobs to this destroyed entity: drivers/gpu/drm/imagination/pvr_job.c:pvr_submit_jobs() { ... for (i = 0; i < args->jobs_count; i++) { pvr_queue_job_arm(jobs[i]); } ... } pvr_queue_job_arm() -> drm_sched_job_arm() will access the freed entity->stats via drm_sched_entity_stats_get(), causing a use-after-free. Additionally, the pushed jobs are permanently leaked, which in turn leaks the context they hold a reference to. Should the entity destruction be deferred until the context reference count reaches zero? > > if (WARN_ON(queue->last_queued_job_scheduled_fence)) > dma_fence_put(queue->last_queued_job_scheduled_fence); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
