On Fri, Sep 20, 2024 at 07:07:36PM +0000, Matthew Brost wrote: > On Fri, Sep 20, 2024 at 01:38:07PM +0100, Matthew Auld wrote: > > The initial kref from dma_fence_init() should match up with whatever > > signals the fence, however here we are submitting the job first to the > > hw and only then grabbing the extra ref and even then we touch some > > fence state before this. This might be too late if the fence is > > signalled before we can grab the extra ref. Rather always grab the > > refcount early before we do the submission part. > > > > I think I see the race. Let me make sure I understand. > > Current flow: > > 1. guc_exec_queue_run_job enters > 2. guc_exec_queue_run_job submits job to hardware > 3. job finishes on hardware > 4. irq handler for job completion fires, signals job->fence, does last > put on job->fence freeing the memory > 5. guc_exec_queue_run_job takes a ref job->fence and BOOM UAF > > The extra ref between steps 1/2 dropped after 5 prevents this. Is that > right? > > Assuming my understanding is correct: > Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx> > Revoking this RB. I should've looked a bit deeper here before the initial reply, but I think I have this fixed here [1]. Let me know if that patch makes sense or if you have have any questions, concerns, or comments. Matt [1] https://patchwork.freedesktop.org/patch/615304/?series=138939&rev=1 > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2811 > > Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx> > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") > > Cc: Matthew Brost <matthew.brost@xxxxxxxxx> > > Cc: <stable@xxxxxxxxxxxxxxx> # v6.8+ > > --- > > drivers/gpu/drm/xe/xe_guc_submit.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c > > index fbbe6a487bbb..b33f3d23a068 100644 > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c > > @@ -766,12 +766,15 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job) > > struct xe_guc *guc = exec_queue_to_guc(q); > > struct xe_device *xe = guc_to_xe(guc); > > bool lr = xe_exec_queue_is_lr(q); > > + struct dma_fence *fence; > > > > xe_assert(xe, !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) || > > exec_queue_banned(q) || exec_queue_suspended(q)); > > > > trace_xe_sched_job_run(job); > > > > + dma_fence_get(job->fence); > > + > > if (!exec_queue_killed_or_banned_or_wedged(q) && !xe_sched_job_is_error(job)) { > > if (!exec_queue_registered(q)) > > register_exec_queue(q); > > @@ -782,12 +785,16 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job) > > > > if (lr) { > > xe_sched_job_set_error(job, -EOPNOTSUPP); > > - return NULL; > > + fence = NULL; > > } else if (test_and_set_bit(JOB_FLAG_SUBMIT, &job->fence->flags)) { > > - return job->fence; > > + fence = job->fence; > > } else { > > - return dma_fence_get(job->fence); > > + fence = dma_fence_get(job->fence); > > } > > + > > + dma_fence_put(job->fence); > > + > > + return fence; > > } > > > > static void guc_exec_queue_free_job(struct drm_sched_job *drm_job) > > -- > > 2.46.0 > >
