Re: [PATCH] drm/xe: Take ref to VM in delayed snapshot

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Jul 26, 2024 at 02:17:04AM -0600, Upadhyay, Tejas wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > Matthew Brost
> > Sent: Friday, July 26, 2024 4:54 AM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>;
> > maarten.lankhorst@linux.intel.com
> > Subject: [PATCH] drm/xe: Take ref to VM in delayed snapshot
> > 
> > Kernel BO's don't take a ref to the VM, we need to the VM for the delayed
> > snapshot, so take a ref to the VM in delayed snapshot.
> 
> Can you please give some details why VM needed now!
> 

If the BO has a VM it shares a dma-resv lock with the VM, thus if the VM
is freed before the TTM object individualized step of a BO we get a UAF.

Ideally we want kernel BOs with a VM to take ref to the VM in a BO layer
(see __xe_bo_create_locked) but there is circular dependency for kernel
BO and VMs which need to get worked out. I'll look into fix that when I
have sometime.

But in meantime this will fix a UAF so it would be good to get in.

Matt

> Tejas
> > 
> > 47058633d9c5 ("drm/xe: Move lrc snapshot capturing to xe_lrc.c")
> > Cc: Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_lrc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c index
> > 94ff62e1d95e..9ef9b45a3596 100644
> > --- a/drivers/gpu/drm/xe/xe_lrc.c
> > +++ b/drivers/gpu/drm/xe/xe_lrc.c
> > @@ -1634,6 +1634,8 @@ struct xe_lrc_snapshot
> > *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
> >  	if (!snapshot)
> >  		return NULL;
> > 
> > +	xe_vm_get(lrc->bo->vm);
> > +
> >  	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
> >  	snapshot->indirect_context_desc =
> > xe_lrc_indirect_ring_ggtt_addr(lrc);
> >  	snapshot->head = xe_lrc_ring_head(lrc); @@ -1653,12 +1655,14 @@
> > struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)  void
> > xe_lrc_snapshot_capture_delayed(struct xe_lrc_snapshot *snapshot)  {
> >  	struct xe_bo *bo;
> > +	struct xe_vm *vm;
> >  	struct iosys_map src;
> > 
> >  	if (!snapshot)
> >  		return;
> > 
> >  	bo = snapshot->lrc_bo;
> > +	vm = bo->vm;
> >  	snapshot->lrc_bo = NULL;
> > 
> >  	snapshot->lrc_snapshot = kvmalloc(snapshot->lrc_size, GFP_KERNEL);
> > @@ -1678,6 +1682,7 @@ void xe_lrc_snapshot_capture_delayed(struct
> > xe_lrc_snapshot *snapshot)
> >  	xe_bo_unlock(bo);
> >  put_bo:
> >  	xe_bo_put(bo);
> > +	xe_vm_put(vm);
> >  }
> > 
> >  void xe_lrc_snapshot_print(struct xe_lrc_snapshot *snapshot, struct
> > drm_printer *p)
> > --
> > 2.34.1
>