Re: [PATCH v3 1/2] drm/sched: fix use-after-free of the fence timeline name

[Jonghyuk Kim(MalHyuk)]

Thanks a lot for the thorough review, and for pulling in the pvr folks.

First, the important one. You flagged the ops-detach as dangerous, and after
your and the bot's pointers I agree it is not viable as-is:

 - amdgpu dereferences the helper unconditionally, e.g.
   amdgpu_cs_p2_dependencies() and amdgpu_ctx_fence_time() do
   to_drm_sched_fence(fence) and then touch ->scheduled without a NULL check.
   Once the finished fence detaches its ops on signalling, to_drm_sched_fence()
   returns NULL for it, so this is a deterministic NULL deref an unprivileged
   process can reach by submitting and then referencing completed jobs. That is
   the bot's [Critical], and it checks out.
 - pvr is worse in the way you described: pvr_queue_fence_is_native() uses the
   ops pointer as an *identity* test, so detaching ops makes it race between
   "native" and "foreign" for one and the same fence.

So detaching the ops breaks the "identify a drm_sched_fence by its ops"
contract that these drivers rely on, and papering over it would mean auditing
every to_drm_sched_fence() caller. I don't think that is the right trade for a
fix we want to backport.

Christian's point that the finished/scheduled .release callbacks are
"unproblematic for the problem at hand" matches this: the release callbacks do
not need to be removed to fix the timeline-name UAF, so keeping them (and thus
the ops attached, and to_drm_sched_fence() working) is fine.

Given that, I'd like to fall back to the minimal caching fix and drop the
ops/refcount rework entirely:

 - get_timeline_name() caches the name in drm_sched_fence_init() and returns
   the cached value, so it never dereferences ->sched. Everything else - both
   .release callbacks, the shared allocation, the call_rcu() free,
   to_drm_sched_fence() - stays exactly as today, so there is no amdgpu/pvr
   regression and nothing new for the backend to reason about.
 - This also addresses Christian's point that the reference must go from the
   finished to the scheduled fence, not the other way around: the caching fix
   keeps the existing finished->scheduled reference untouched and does not
   invert it, so the finished->scheduled conversions that rely on that keep
   working.
 - This makes most of the per-patch comments on v3 (the shared-allocation
   lifetime, the extra dma_fence_get(), the "last put" wording, moving
   call_rcu) moot, since that rework goes away. I'll keep the ones that still
   apply.

On the specific points:

 - get_driver_name(): it returns the literal "drm_sched" and never touches
   ->sched, so unlike get_timeline_name() it isn't exposed. Only the timeline
   name needs the fix.
 - The "already-satisfied dependency / dependency-collapsing" wording and the
   whole to_drm_sched_fence()-returns-NULL discussion only existed to justify
   the ops-detach; with caching, to_drm_sched_fence() keeps working as today,
   so that reasoning (and the confusion around it) goes away entirely.
 - Caching only the pointer: the earlier objection was that it doesn't help
   drivers whose name is freed together with the scheduler. The mainline
   drivers that actually hit this (amdxdna, nouveau, msm VM_BIND) pass a name
   that lives as long as the scheduler, and panthor/xe (dynamically allocated
   names) are already fixed per-driver. If you'd rather close the dynamic-name
   case generically in the core too, I can kstrdup() the name into the fence at
   init and free it on fence release - one small alloc per fence. I'm happy to
   go pointer-cache or kstrdup, whichever you and Tvrtko prefer.
 - Cc: stable: will add "Cc: stable@xxxxxxxxxxxxxxx # we don't know since when"
   and let the stable folks pick the backport depth, as you suggested.
 - Whitespace/doc reflow: will split into its own patch and keep the fix patch
   free of unrelated formatting churn.
 - kmemleak: the caching fix doesn't change any refcounts, but I'll re-run the
   KUnit suite under kmemleak as well as KASAN before resending.

Unless someone would prefer to keep ops-detach and fix the two callers instead,
I'll respin as the caching v4 once Tvrtko and the pvr folks have had a chance to
look as well.

Thanks again,
Jonghyuk