linux-kernel - Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a3b3e8d2-ada8-4b34-91d4-e9dc22b83b46@ursulin.net>
Date: Mon, 12 May 2025 08:52:29 +0100
From: Tvrtko Ursulin <tursulin@...ulin.net>
To: Rob Clark <robdclark@...il.com>, dri-devel@...ts.freedesktop.org
Cc: Rob Clark <robdclark@...omium.org>,
 Matthew Brost <matthew.brost@...el.com>, Danilo Krummrich <dakr@...nel.org>,
 Philipp Stanner <phasta@...nel.org>,
 Christian König <ckoenig.leichtzumerken@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()


On 09/05/2025 22:29, Rob Clark wrote:
> From: Rob Clark <robdclark@...omium.org>
> 
> The fence can outlive the sched, so it is not safe to dereference the
> sched in drm_sched_fence_get_timeline_name()

Funny I've been working in the same problem space:

See 
https://lore.kernel.org/dri-devel/20250509153352.7187-1-tvrtko.ursulin@igalia.com/

> 
> Signed-off-by: Rob Clark <robdclark@...omium.org>
> ---
>   drivers/gpu/drm/scheduler/sched_fence.c |  3 ++-
>   include/drm/gpu_scheduler.h             | 11 +++++++++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index e971528504a5..4e529c3ba6d4 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -92,7 +92,7 @@ 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;
> +	return fence->name;
>   }
>   
>   static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
> @@ -226,6 +226,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
>   	unsigned seq;
>   
>   	fence->sched = entity->rq->sched;
> +	fence->name  = fence->sched->name;
>   	seq = atomic_inc_return(&entity->fence_seq);
>   	dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
>   		       &fence->lock, entity->fence_context, seq);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 0ae108f6fcaf..d830ffe083f1 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -295,6 +295,9 @@ struct drm_sched_fence {
>           /**
>            * @sched: the scheduler instance to which the job having this struct
>            * belongs to.
> +         *
> +         * Some care must be taken as to where the sched is derefed, as the
> +         * fence can outlive the sched.
>            */
>   	struct drm_gpu_scheduler	*sched;
>           /**
> @@ -305,6 +308,14 @@ struct drm_sched_fence {
>            * @owner: job owner for debugging
>            */
>   	void				*owner;
> +
> +	/**
> +	 * @name: the timeline name
> +	 *
> +	 * This comes from the @sched, but since the fence can outlive the
> +	 * sched, we need to keep our own copy.
> +	 */
> +	const char			*name;

With drivers such as xe, fence->sched can indeed be freed, but so can 
sched->name, so it is not safe to store a copy of it. AFAICT only safe 
way is to simply give up on the real names for signalled fences.

Could you see if my series fixes the issue in your use case? I *think* 
by using the driver/timeline name wrappers I did catch all external 
access points and made them safe.

Regards,

Tvrtko

>   };
>   
>   struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);