[PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()

[Rob Clark]

On Mon, May 12, 2025 at 12:54 AM Danilo Krummrich <dakr at kernel.org> wrote:
>
> On Fri, May 09, 2025 at 02:29:36PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark at chromium.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 at chromium.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;
> >  };