Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free

[Christian König]

On 9/4/26 10:06, Jonghyuk Kim(MalHyuk) wrote:
> drm_sched_fence_get_timeline_name() returns fence->sched->name, and the
> drm_sched_fence ops keep a .release callback, so the fence is not
> ops-detached on signalling (dma_fence_signal_timestamp_locked() only
> clears ->ops for fences without .release/.wait). The callback therefore
> stays reachable on a long-signalled, userspace-held finished fence and
> unconditionally dereferences fence->sched.
> 
> A driver that allocates a drm_gpu_scheduler at per-context/per-queue/per-VM
> granularity and frees it on an unprivileged context/fd close, while
> exporting the resulting finished fence to userspace (drm_syncobj /
> sync_file / dma_resv), leaves fence->sched dangling after the free. A
> subsequent SYNC_IOC_FILE_INFO ioctl (which calls get_timeline_name()) then
> reads the freed scheduler:
> 
>   BUG: KASAN: slab-use-after-free in drm_sched_fence_get_timeline_name
> 
> This is the same class as CVE-2025-38703 (drm/xe) and CVE-2025-71302
> (drm/panthor), which were fixed per-driver. amdxdna, nouveau and msm
> (VM_BIND) are still affected in mainline, so fix it in the core to cover
> any per-context-scheduler driver at once.
> 
> Cache the scheduler's name pointer in the fence at init time, while the
> scheduler is guaranteed alive, and return the cached value from
> get_timeline_name() without dereferencing fence->sched. The timeline name
> is not guaranteed by the contract to outlive the scheduler, so document in
> struct drm_sched_init_args that the @name passed to drm_sched_init() must
> follow the dma-fence safe access rules and outlive any exported fence.
> Every in-tree driver passes a string literal, which satisfies this;
> commit 299bc6d50b1b ("drm/xe/guc: Keep scheduler timeline name alive")
> keeps drm/xe's dynamically-allocated name alive across the RCU grace and
> can be simplified on top of this.
> 
> Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the rules")
> Cc: stable@xxxxxxxxxxxxxxx # we don't know since when
> Signed-off-by: Jonghyuk Kim(MalHyuk) <malhyuk97@xxxxxxxxx>
> ---
>  drivers/gpu/drm/scheduler/sched_fence.c | 24 +++++++++++++++++++++++-
>  include/drm/gpu_scheduler.h             | 18 +++++++++++++++++-
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index 096fe28aa9c9..b2a842a1c9ba 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -92,7 +92,13 @@ static const char *drm_sched_fence_get_driver_name(struct dma_fence *fence)
>  static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
>  {
>  	struct drm_sched_fence *fence = to_drm_sched_fence(f);
> -	return (const char *)fence->sched->name;
> +
> +	/*
> +	 * Do not dereference fence->sched here: a userspace-held finished
> +	 * fence can outlive a per-context scheduler. Return the name cached
> +	 * in drm_sched_fence_init() instead.
> +	 */
> +	return fence->sched_name;

I don't think that this actually solves the problem, the sched_name still needs to be kept alive until all fences are destroyed and that is something drivers don't want/can do.

>  }
>  
>  static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
> @@ -180,6 +186,14 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
>  		dma_fence_set_deadline(parent, deadline);
>  }
>  
> +/*
> + * TODO: Both fences implement .release, so dma_fence keeps their ops attached
> + * after signalling. Dropping the callbacks would let dma_fence detach the ops,
> + * after which neither get_timeline_name() nor get_driver_name() can run against
> + * a freed scheduler or an unloaded module - the complete fix. It first requires
> + * auditing every to_drm_sched_fence() caller, since ops-detach makes the helper
> + * return NULL for a signalled fence. See Documentation/gpu/todo.rst.
> + */

That sounds like a bad idea as well.

Dropping the fence->ops is to detach the fence from the module which originally issued it and not solve lifetime problems between the scheduler and the driver.

I think we should rather re-consider patch 035219a760edb35ae9a9e96beba7f122e26a997b ("dma-buf: dma-fence: Fix potential NULL pointer dereference"):

Here we changed the check in dma_fence_driver_name() and dma_fence_timeline_name():

@@ -1167,7 +1167,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
 
        /* RCU protection is required for safe access to returned string */
        ops = rcu_dereference(fence->ops);
-       if (!dma_fence_test_signaled_flag(fence))
+       if (ops)
                return (const char __rcu *)ops->get_driver_name(fence);
        else
                return (const char __rcu *)"detached-driver";

The problem is that we didn't considered that there a fence implementations which still have a release or wait callbacks but rely on not needing to return a string for a signaled fence.

Regards,
Christian.