<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body>
    Am 30.08.24 um 23:43 schrieb Matthew Brost:<br>
    <blockquote type="cite" cite="mid:ZtI9EMzHZW3DkHw%2F@DUT025-TGLU.fm.intel.com">
      <pre class="moz-quote-pre" wrap="">On Fri, Aug 30, 2024 at 10:14:18AM +0200, Christian König wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Am 29.08.24 um 19:12 schrieb Boris Brezillon:
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">dma_fence objects created by an entity might outlive the
drm_gpu_scheduler this entity was bound to if those fences are retained
by other other objects, like a dma_buf resv. This means that
drm_sched_fence::sched might be invalid when the resv is walked, which
in turn leads to a UAF when dma_fence_ops::get_timeline_name() is called.

This probably went unnoticed so far, because the drm_gpu_scheduler had
the lifetime of the drm_device, so, unless you were removing the device,
there were no reasons for the scheduler to be gone before its fences.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
Nope, that is intentional design. get_timeline_name() is not safe to be
called after the fence signaled because that would causes circular
dependency problems.

</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
I'm quite sure happens, ftrace for example can and will call
get_timeline_name in trace_dma_fence_destroy which is certainly after
the fence is signaled. There are likely other cases too - this just
quickly came to mind.</pre>
    </blockquote>
    <br>
    Good point, completely forgotten about ftrace.<br>
    <br>
    <span style="white-space: pre-wrap">
</span>
    <blockquote type="cite" cite="mid:ZtI9EMzHZW3DkHw%2F@DUT025-TGLU.fm.intel.com">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">E.g. when you have hardware fences it can happen that fences reference a
driver module (for the function printing the name) and the module in turn
keeps fences around.

</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
I am almost positive without this patch this problematic in Xe or any
driver in which schedulers are tied to IOCTLs rather than kernel module.

In Xe 'fence->sched' maps to an xe_exec_queue which can be freed once
the destroy exec queue IOCTL is called and all jobs are free'd (i.e.
'fence' signals). The fence could be live on after in dma-resv objects,
drm syncobjs, etc...

I know this issue has been raised before and basically NACK'd but I have
a strong opinion this is valid and in fact required.</pre>
    </blockquote>
    <br>
    I've NACK'd automatically signaling pending fences on destruction of
    the scheduler (that reminds me that I wanted to add a warning for
    that) and copying the name into every scheduler fence.<br>
    <br>
    As long as we don't do any of that I'm perfectly fine fixing this
    issue. The approach of creating a reference counted object for the
    name looks rather valid to me.<br>
    <br>
    Especially since we then pretty much get the module references
    correct for free as well.<br>
    <br>
    Christian.<br>
    <br>
    <blockquote type="cite" cite="mid:ZtI9EMzHZW3DkHw%2F@DUT025-TGLU.fm.intel.com">
      <pre class="moz-quote-pre" wrap="">

Matt

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">So you easily end up with a module you can never unload.


</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">With the introduction of a new model where each entity has its own
drm_gpu_scheduler instance, this situation is likely to happen every time
a GPU context is destroyed and some of its fences remain attached to
dma_buf objects still owned by other drivers/processes.

In order to make drm_sched_fence_get_timeline_name() safe, we need to
copy the scheduler name into our own refcounted object that's only
destroyed when both the scheduler and all its fences are gone.

The fact drm_sched_fence might have a reference to the drm_gpu_scheduler
even after it's been released is worrisome though, but I'd rather
discuss that with everyone than come up with a solution that's likely
to end up being rejected.

Note that the bug was found while repeatedly reading dma_buf's debugfs
file, which, at some point, calls dma_resv_describe() on a resv that
contains signalled fences coming from a destroyed GPU context.
AFAIK, there's nothing invalid there.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
Yeah but reading debugfs is not guaranteed to crash the kernel.

On the other hand the approach with a kref'ed string looks rather sane to
me. One comment on this below.

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
Cc: Luben Tuikov <a class="moz-txt-link-rfc2396E" href="mailto:ltuikov89@gmail.com"><ltuikov89@gmail.com></a>
Cc: Matthew Brost <a class="moz-txt-link-rfc2396E" href="mailto:matthew.brost@intel.com"><matthew.brost@intel.com></a>
Cc: "Christian König" <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a>
Cc: Danilo Krummrich <a class="moz-txt-link-rfc2396E" href="mailto:dakr@redhat.com"><dakr@redhat.com></a>
Signed-off-by: Boris Brezillon <a class="moz-txt-link-rfc2396E" href="mailto:boris.brezillon@collabora.com"><boris.brezillon@collabora.com></a>
---
  drivers/gpu/drm/scheduler/sched_fence.c |  8 +++-
  drivers/gpu/drm/scheduler/sched_main.c  | 18 ++++++++-
  include/drm/gpu_scheduler.h             | 52 +++++++++++++++++++++++++
  3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 0f35f009b9d3..efa2a71d98eb 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -90,7 +90,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 (const char *)fence->timeline->name;
  }
  static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
@@ -112,8 +112,10 @@ static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
   */
  void drm_sched_fence_free(struct drm_sched_fence *fence)
  {
+       drm_sched_fence_timeline_put(fence->timeline);
+
        /* This function should not be called if the fence has been initialized. */
-       if (!WARN_ON_ONCE(fence->sched))
+       if (!WARN_ON_ONCE(fence->timeline))
                kmem_cache_free(sched_fence_slab, fence);
  }
@@ -224,6 +226,8 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
        unsigned seq;
        fence->sched = entity->rq->sched;
+       fence->timeline = fence->sched->fence_timeline;
+       drm_sched_fence_timeline_get(fence->timeline);
        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/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 7e90c9f95611..1e31a9c8ce15 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1288,10 +1288,21 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
                sched->own_submit_wq = true;
        }
+       sched->fence_timeline = kzalloc(sizeof(*sched->fence_timeline), GFP_KERNEL);
+       if (!sched->fence_timeline)
+               goto Out_check_own;
+
+       sched->fence_timeline->name = kasprintf(GFP_KERNEL, "%s", sched->name);
+       if (!sched->fence_timeline->name)
+               goto Out_free_fence_timeline;
+
+       kref_init(&sched->fence_timeline->kref);
+
        sched->sched_rq = kmalloc_array(num_rqs, sizeof(*sched->sched_rq),
                                        GFP_KERNEL | __GFP_ZERO);
        if (!sched->sched_rq)
-               goto Out_check_own;
+               goto Out_free_fence_timeline;
+
        sched->num_rqs = num_rqs;
        for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
                sched->sched_rq[i] = kzalloc(sizeof(*sched->sched_rq[i]), GFP_KERNEL);
@@ -1319,6 +1330,10 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
        kfree(sched->sched_rq);
        sched->sched_rq = NULL;
+Out_free_fence_timeline:
+       if (sched->fence_timeline)
+               kfree(sched->fence_timeline->name);
+       kfree(sched->fence_timeline);
  Out_check_own:
        if (sched->own_submit_wq)
                destroy_workqueue(sched->submit_wq);
@@ -1367,6 +1382,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
        sched->ready = false;
        kfree(sched->sched_rq);
        sched->sched_rq = NULL;
+       drm_sched_fence_timeline_put(sched->fence_timeline);
  }
  EXPORT_SYMBOL(drm_sched_fini);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 5acc64954a88..615ca89f77dc 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -261,6 +261,52 @@ struct drm_sched_rq {
        struct rb_root_cached           rb_tree_root;
  };
+/**
+ * struct drm_sched_fence_timeline - Wrapped around the timeline name
+ *
+ * This is needed to cope with the fact dma_fence objects created by
+ * an entity might outlive the drm_gpu_scheduler this entity was bound
+ * to, making drm_sched_fence::sched invalid and leading to a UAF when
+ * dma_fence_ops::get_timeline_name() is called.
+ */
+struct drm_sched_fence_timeline {
+       /** @kref: Reference count of this timeline object. */
+       struct kref                     kref;
+
+       /**
+        * @name: Name of the timeline.
+        *
+        * This is currently a copy of drm_gpu_scheduler::name.
+        */
+       const char                      *name;
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
Make that a char name[] and embed the name into the structure. The macro
struct_size() can be used to calculate the size.

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+};
+
+static inline void
+drm_sched_fence_timeline_release(struct kref *kref)
+{
+       struct drm_sched_fence_timeline *tl =
+               container_of(kref, struct drm_sched_fence_timeline, kref);
+
+       kfree(tl->name);
+       kfree(tl);
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
This avoids having two allocations for the timeline name.

Regards,
Christian.

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+}
+
+static inline void
+drm_sched_fence_timeline_put(struct drm_sched_fence_timeline *tl)
+{
+       if (tl)
+               kref_put(&tl->kref, drm_sched_fence_timeline_release);
+}
+
+static inline struct drm_sched_fence_timeline *
+drm_sched_fence_timeline_get(struct drm_sched_fence_timeline *tl)
+{
+       if (tl)
+               kref_get(&tl->kref);
+
+       return tl;
+}
+
  /**
   * struct drm_sched_fence - fences corresponding to the scheduling of a job.
   */
@@ -289,6 +335,9 @@ struct drm_sched_fence {
         */
        ktime_t                         deadline;
+        /** @timeline: the timeline this fence belongs to. */
+       struct drm_sched_fence_timeline *timeline;
+
          /**
           * @parent: the fence returned by &drm_sched_backend_ops.run_job
           * when scheduling the job on hardware. We signal the
@@ -488,6 +537,8 @@ struct drm_sched_backend_ops {
   * @credit_count: the current credit count of this scheduler
   * @timeout: the time after which a job is removed from the scheduler.
   * @name: name of the ring for which this scheduler is being used.
+ * @fence_timeline: fence timeline that will be passed to fences created by
+ *                  and entity that's bound to this scheduler.
   * @num_rqs: Number of run-queues. This is at most DRM_SCHED_PRIORITY_COUNT,
   *           as there's usually one run-queue per priority, but could be less.
   * @sched_rq: An allocated array of run-queues of size @num_rqs;
@@ -521,6 +572,7 @@ struct drm_gpu_scheduler {
        atomic_t                        credit_count;
        long                            timeout;
        const char                      *name;
+       struct drm_sched_fence_timeline *fence_timeline;
        u32                             num_rqs;
        struct drm_sched_rq             **sched_rq;
        wait_queue_head_t               job_scheduled;
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
</pre>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>