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] [day] [month] [year] [list]

Message-ID: <CAF6AEGtSr0Y7nk2Jrk+yzoxnW8WGs5S9iOVtvxfQ1hcS9e0AtA@mail.gmail.com>
Date: Mon, 12 May 2025 07:57:00 -0700
From: Rob Clark <robdclark@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: dri-devel@...ts.freedesktop.org, Rob Clark <robdclark@...omium.org>, 
	Matthew Brost <matthew.brost@...el.com>, 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>, Tvrtko Ursulin <tursulin@...ulin.net>
Subject: Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()

On Mon, May 12, 2025 at 12:54 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Fri, May 09, 2025 at 02:29:36PM -0700, 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()
> >
> > 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;
>
> This requires sched->name to be a string in the .(ro)data section of the binary,
> or a string that the driver only ever frees after all fences of this scheduler
> have been freed.
>
> Are we sure that those rules are documented and honored by existing drivers?
>
> Otherwise, we might just fix one bug and create a more subtle one instead. :(

Agreed, but at least it is _less_ bad, and the alternative of
refcnting the sched seemed pretty heavy handed :-(

I'll take a look at Tvrtko's series to see if that could help.

I suppose the alternative is to null out the sched ptr when the fence
is signalled, and then return some generic name (ie "signalled" or
something like that)?

BR,
-R

>
> >       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.
>
> It's our own copy of the pointer, not our own copy of the string. I think we
> should be clear about that.
>
> > +      */
> > +     const char                      *name;
> >  };