Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Dereference of ERR_PTR in panthor_heap_pool_put() during tiler OOM handling. - [High] Asynchronous cleanup of scheduling groups executes without holding a DRM device reference, leading to a Use-After-Free of panthor_device. - [High] Missing bounds check on synchronization object offset allows out-of-bounds read. -- commit eccb99f3f3029758d8c3f80d0f349c280e91b3c2 Author: Boris Brezillon <[email protected]> drm/panthor: Don't overrule pending immediate ticks in sched_resume_tick() We schedule immediate ticks when we need to process events on CSGs, but those immediate ticks don't change the resched_target because we want the other groups to stay scheduled for the remaining of the GPU timeslot they were given. Make sure these immediate ticks don't get overruled by a sched_queue_delayed_work() that would delay the tick execution. Fixes: 99820b4b7e50 ("drm/panthor: Make sure we resume the tick when new jobs are submitted") Reported-by: [email protected] Closes: https://sashiko.dev/#/patchset/[email protected]?part=9 > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > index 5b34032deff81..1913bc8a62977 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -2668,7 +2668,14 @@ static void sched_resume_tick(struct panthor_device > *ptdev) > else > delay_jiffies = 0; > > - sched_queue_delayed_work(sched, tick, delay_jiffies); > + /* We schedule immediate ticks when we need to process events on CSGs, > + * but those don't change the resched_target because we want the other > + * groups to stay scheduled for the remaining of the GPU timeslot they > + * were given. Make sure those immediate ticks don't get overruled by > + * a sched_queue_delayed_work() that would delay the tick execution. > + */ > + if (!delayed_work_pending(&sched->tick_work)) > + sched_queue_delayed_work(sched, tick, delay_jiffies); > } [Severity: High] This isn't a bug introduced by this patch, but we noticed a potential out-of-bounds read nearby. In panthor_queue_get_syncwait_obj(), the return value of panthor_gem_sync() is ignored: drivers/gpu/drm/panthor/panthor_sched.c:panthor_queue_get_syncwait_obj() { ... panthor_gem_sync(&bo->base, DRM_PANTHOR_BO_SYNC_CPU_CACHE_FLUSH_AND_INVALIDATE, queue->syncwait.offset, queue->syncwait.sync64 ? sizeof(struct panthor_syncobj_64b) : sizeof(struct panthor_syncobj_32b)); return queue->syncwait.kmap + queue->syncwait.offset; } If panthor_gem_sync() detects the offset is out of bounds and returns -EINVAL, the function ignores the error and still returns the pointer, which points outside the buffer boundary. Could this allow panthor_queue_eval_syncwait() to dereference an out-of-bounds pointer when it reads syncobj->sync64.seqno? [Severity: High] This is a pre-existing issue, but is it safe for group_release() to queue asynchronous cleanup work without pinning the DRM device? drivers/gpu/drm/panthor/panthor_sched.c:group_release() { ... queue_work(panthor_cleanup_wq, &group->release_work); } When the DRM device file is closed and panthor_postclose() executes group destruction, it doesn't flush the panthor_cleanup_wq workqueue. If the device is hot-unplugged, the DRM core might free the panthor_device. When group_release_work() eventually executes later, does this lead to a use-after-free when it accesses the freed panthor_device during cleanup (e.g., in panthor_kernel_bo_destroy())? [Severity: High] This is a pre-existing issue, but we noticed a potential pointer dereference issue in the tiler OOM handling path. In group_process_tiler_oom(): drivers/gpu/drm/panthor/panthor_sched.c:group_process_tiler_oom() { ... heaps = panthor_vm_get_heap_pool(group->vm, false); ... if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) { ret = -EINVAL; ... out_put_heap_pool: panthor_heap_pool_put(heaps); return ret; } If heaps is an ERR_PTR (like ERR_PTR(-ENOENT)), the code correctly sets ret = -EINVAL but later jumps to out_put_heap_pool and calls panthor_heap_pool_put(). Since panthor_heap_pool_put() only checks if (pool) and doesn't check IS_ERR(pool), wouldn't this try to dereference an invalid error pointer in kref_put()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
