Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] Unconditional dereference of `to_drm_sched_fence()` return value causes a deterministic kernel crash for signaled finished fences in downstream drivers. - [High] TOCTOU race during ops detachment causes `to_drm_sched_fence()` to return NULL, leading to a NULL pointer dereference in ops callbacks. Pre-existing issues: - [High] The scheduled fence retains its `.release` callback, leaving it vulnerable to the same use-after-free of the scheduler timeline name if it outlives the scheduler. -- commit cbf4f1124a22fb62bedbf4d0c94b024440ce5beb Author: Jonghyuk Kim(MalHyuk) <[email protected]> drm/sched: fix use-after-free of the fence timeline name This commit aims to resolve a use-after-free vulnerability involving the fence timeline name by removing the .release callback from the finished fence. This allows dma_fence to detach the fence operations on signaling, while restructuring references so the scheduled fence holds a reference to the finished fence. > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c > b/drivers/gpu/drm/scheduler/sched_fence.c > --- a/drivers/gpu/drm/scheduler/sched_fence.c > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > @@ -95,15 +95,6 @@ static const char > *drm_sched_fence_get_timeline_name(struct dma_fence *f) > return (const char *)fence->sched->name; [Severity: High] Can a time-of-check to time-of-use race occur here leading to a null pointer dereference? Code paths like dma_fence_timeline_name() locklessly read f->ops under RCU. If another thread concurrently signals the fence, dma_fence_signal() will detach the ops by setting f->ops to NULL. If the first thread reads the valid &drm_sched_fence_ops_finished before the detachment, but calls ops->get_timeline_name(fence) after, to_drm_sched_fence() will re-read f->ops (which is now NULL) and return NULL. Since fence is unconditionally dereferenced in the return statement above, and similarly in drm_sched_fence_set_deadline_finished() for the deadline field, could this race window cause a regression if hit? Userspace could potentially trigger this by invoking SYNC_IOC_FILE_INFO while a job completes. > } [ ... ] > @@ -189,7 +171,13 @@ static const struct dma_fence_ops > drm_sched_fence_ops_scheduled = { [Severity: High] This isn't a bug introduced by this patch, but does the scheduled fence remain vulnerable to the same use-after-free issue? This patch removes the .release callback from drm_sched_fence_ops_finished to allow ops detachment on signaling. However, drm_sched_fence_ops_scheduled retains its .release callback (drm_sched_fence_release_scheduled). Because it has a .release callback, dma_fence_signal() will not detach the ops for the scheduled fence. If a subsystem holds a reference to the scheduled fence after the scheduler is destroyed, won't any subsequent timeline name queries dereference the dangling fence->sched->name pointer, triggering the same slab-use-after-free this patch addresses? > static const struct dma_fence_ops drm_sched_fence_ops_finished = { > .get_driver_name = drm_sched_fence_get_driver_name, > .get_timeline_name = drm_sched_fence_get_timeline_name, > - .release = drm_sched_fence_release_finished, > + /* > + * No .release callback: dma_fence detaches ->ops on signalling for > + * fences without .release/.wait, so get_timeline_name() is never called > + * on a signalled finished fence and cannot dereference a freed > + * scheduler. The shared allocation is freed from dma_fence_free() once > + * this fence's refcount drops - it is released last, after @scheduled. > + */ [Severity: Critical] Does removing the .release callback here cause a regression in downstream drivers, resulting in a deterministic crash? Because the ops are now detached on signaling (setting f->ops to NULL), to_drm_sched_fence() will return NULL for a signaled finished fence. However, downstream drivers assume their scheduler fences are always recognized and unconditionally dereference the result of to_drm_sched_fence(). For example, in amdgpu_cs_p2_dependencies(): drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_p2_dependencies() { ... s_fence = to_drm_sched_fence(fence); fence = dma_fence_get(&s_fence->scheduled); ... } And in amdgpu_ctx_fence_time(): drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c:amdgpu_ctx_fence_time() { ... s_fence = to_drm_sched_fence(fence); if (!test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &s_fence->scheduled.flags)) ... } When an unprivileged user closes a context with completed jobs or queries dependencies on completed jobs, won't this result in an immediate null pointer dereference? > .set_deadline = drm_sched_fence_set_deadline_finished, > }; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
