Re: [PATCH v3 1/2] drm/sched: fix use-after-free of the fence timeline name

[sashiko-bot@kernel.org]

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) <malhyuk97@gmail.com>
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/20260902144204.1843670-1-malhyuk97@gmail.com?part=1