* Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()
2025-05-09 21:29 [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name() Rob Clark
@ 2025-05-12 7:32 ` Philipp Stanner
2025-05-12 7:52 ` Tvrtko Ursulin
2025-05-12 7:54 ` Danilo Krummrich
2 siblings, 0 replies; 5+ messages in thread
From: Philipp Stanner @ 2025-05-12 7:32 UTC (permalink / raw)
To: Rob Clark, dri-devel
Cc: Rob Clark, Matthew Brost, Danilo Krummrich, Philipp Stanner,
Christian König, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, open list
Hi,
On Fri, 2025-05-09 at 14:29 -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> The fence can outlive the sched, so it is not safe to dereference the
> sched in drm_sched_fence_get_timeline_name()
Thx for the fix. Looks correct to me. Some nits
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
This is clearly a bug. So please provide a Fixes: tag and +Cc the
stable kernel.
> ---
> 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;
Adding an empty line while we're here already would be neat.
> }
>
> 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.
> */
Just briefly hinting at the lifetime is enough here. Every developer
understands what that implicates.
"Might have a lifetime shorter than the owning &struct drm_sched_fence"
Thx
P.
> 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;
> };
>
> struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()
2025-05-09 21:29 [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name() Rob Clark
2025-05-12 7:32 ` Philipp Stanner
@ 2025-05-12 7:52 ` Tvrtko Ursulin
2025-05-12 7:54 ` Danilo Krummrich
2 siblings, 0 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2025-05-12 7:52 UTC (permalink / raw)
To: Rob Clark, dri-devel
Cc: Rob Clark, Matthew Brost, Danilo Krummrich, Philipp Stanner,
Christian König, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, open list
On 09/05/2025 22:29, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.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@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;
> 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);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()
2025-05-09 21:29 [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name() Rob Clark
2025-05-12 7:32 ` Philipp Stanner
2025-05-12 7:52 ` Tvrtko Ursulin
@ 2025-05-12 7:54 ` Danilo Krummrich
2025-05-12 14:57 ` Rob Clark
2 siblings, 1 reply; 5+ messages in thread
From: Danilo Krummrich @ 2025-05-12 7:54 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, Rob Clark, Matthew Brost, Philipp Stanner,
Christian König, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, open list
On Fri, May 09, 2025 at 02:29:36PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@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@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. :(
> 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;
> };
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/sched: Fix UAF in drm_sched_fence_get_timeline_name()
2025-05-12 7:54 ` Danilo Krummrich
@ 2025-05-12 14:57 ` Rob Clark
0 siblings, 0 replies; 5+ messages in thread
From: Rob Clark @ 2025-05-12 14:57 UTC (permalink / raw)
To: Danilo Krummrich
Cc: dri-devel, Rob Clark, Matthew Brost, Philipp Stanner,
Christian König, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, open list,
Tvrtko Ursulin
On Mon, May 12, 2025 at 12:54 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri, May 09, 2025 at 02:29:36PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@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@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;
> > };
^ permalink raw reply [flat|nested] 5+ messages in thread