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) <[email protected]> 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/[email protected]?part=1
