Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch attempts to fix a Use-After-Free by caching the timeline name in `fence->sched_name`, but it introduces a direct Use-After-Free of the string memory itself. The commit message incorrectly assumes all drivers pass a string literal or keep the string alive via RCU. However, `panthor` dynamically allocates the timeline name (`kasprintf`) and synchronously frees it (`kfree(queue->name)`) when the queue is destroyed. Furthermore, for `xe`, `kfree_rcu` does not protect against a `sync_file` fd held open by userspace for an arbitrary duration (userspace holding an fd does not equate to holding an RCU read lock). When userspace queries the fence info via the `SYNC_IOC_FILE_INFO` ioctl after the string is freed, `drm_sched_fence_get_timeline_name()` returns the cached pointer to the freed memory, leading to an info leak to userspace.

Pre-existing issues:
- [High] The AMDGPU driver's `amdgpu_sync_same_dev()` function incorrectly assumes that any `drm_sched_fence` belongs to an AMDGPU ring. `to_drm_sched_fence()` returns non-NULL for *any* DRM scheduler fence globally (e.g., from `panthor`, `xe`, `nouveau`). If a foreign fence is passed via a `dma-buf` or `syncobj`, `amdgpu_sync_same_dev()` unconditionally calculates `ring = container_of(s_fence->sched, struct amdgpu_ring, sched)` and dereferences `ring->adev`. This results in an out-of-bounds read (Type Confusion). Additionally, if the foreign fence originated from a per-context scheduler that has been freed (like a destroyed `panthor` queue), this results in a Use-After-Free read.
- [High] The PowerVR driver's `pvr_queue_fence_is_native()` accesses `sched_fence->sched->ops` to determine if a fence is native to the driver. However, if the `drm_sched_fence` originated from a per-context scheduler (whether from `pvr` or another driver like `panthor`) and that scheduler has been destroyed, `sched_fence->sched` is a dangling pointer. Dereferencing it to check `->ops` causes a Use-After-Free read.
--

commit 99163b915ba8e6d549d23c04536ae0f2ac030b70
Author: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>

drm/sched: cache the timeline name to fix a use-after-free

This commit caches the scheduler's timeline name inside drm_sched_fence to
prevent a use-after-free when a userspace-held finished fence outlives a
per-context scheduler. It documents that the name must outlive the fence,
assuming all in-tree drivers pass string literals or keep the name alive.

>     Every in-tree driver passes a string literal, which satisfies this;

[Severity: High]
Is this assumption correct for all in-tree drivers?

For example, panthor dynamically allocates the timeline name and synchronously
frees it when the queue is destroyed:

drivers/gpu/drm/panthor/panthor_sched.c:group_free_queue() {
    ...
    kfree(queue->name);
    ...
}

If a user creates a panthor queue, submits a job to get a fence, exports it
to a sync_file, and destroys the queue, the string memory is freed. When
userspace then queries the fence info via the SYNC_IOC_FILE_INFO ioctl,
wouldn't the cached pointer point to freed memory, leading to an info leak?

> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index 096fe28aa9c9e..b2a842a1c9ba9 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -92,7 +92,13 @@ 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;
> +
> +	/*
> +	 * Do not dereference fence->sched here: a userspace-held finished
> +	 * fence can outlive a per-context scheduler. Return the name cached
> +	 * in drm_sched_fence_init() instead.
> +	 */
> +	return fence->sched_name;

[Severity: High]
Does returning this cached pointer just replace the scheduler use-after-free
with a string memory use-after-free?

Because drivers like panthor can free the string memory synchronously during
queue destruction, this cached pointer will point to freed memory when read
by the caller.

[ ... ]

> @@ -228,6 +242,14 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
>  	unsigned seq;
>  
>  	fence->sched = entity->rq->sched;
> +	/*
> +	 * Cache the scheduler's timeline name. The finished fence may be
> +	 * exported to userspace and outlive @sched (per-context schedulers are
> +	 * freed on context teardown), so get_timeline_name() must not
> +	 * dereference @sched. The name is required to outlive any exported
> +	 * fence (see @name in struct drm_sched_init_args).
> +	 */
> +	fence->sched_name = fence->sched->name;

[Severity: High]
Does caching the string pointer here without taking ownership or extending
its lifetime erroneously assume the string will inherently outlive the fence?

This is a pre-existing issue, but the dangling fence->sched pointer causes
problems in other drivers as well when foreign fences are imported.

[Severity: High]
Does the AMDGPU driver's amdgpu_sync_same_dev() incorrectly assume any
drm_sched_fence globally belongs to an AMDGPU ring?

drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c:amdgpu_sync_same_dev() {
    ...
    ring = container_of(s_fence->sched, struct amdgpu_ring, sched);
    return ring->adev == adev;
}

If a foreign fence from another driver (e.g., panthor) is passed via a dma-buf
or syncobj and used as an input dependency to the AMDGPU command submission
ioctl, this unconditional container_of() calculation will dereference an
invalid or freed pointer. Can this lead to an out-of-bounds type confusion
read, or a use-after-free read if the original per-context scheduler was
already destroyed?

This is also a pre-existing issue, but similar to the AMDGPU issue above,
does the PowerVR driver also dereference a freed scheduler pointer?

[Severity: High]
drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_fence_is_native() {
    ...
    if (sched_fence &&
        sched_fence->sched->ops == &pvr_queue_sched_ops)
    ...
}

If a user exports a drm_sched_fence from a queue, destroys that queue (freeing
the scheduler), and then passes the fence as an input dependency to the PVR
job submission ioctl, sched_fence->sched will be a dangling pointer. Will
dereferencing it to check the ops field cause a use-after-free read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904080618.2098450-1-malhyuk97@gmail.com?part=1