Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier

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

On Tue, Apr 30, 2024 at 09:11:42AM -0600, Zeng, Oak wrote:
> 
> 
> > -----Original Message-----
> > From: Brost, Matthew <matthew.brost@intel.com>
> > Sent: Monday, April 29, 2024 1:18 PM
> > To: Zeng, Oak <oak.zeng@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; Thomas Hellström
> > <thomas.hellstrom@linux.intel.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
> > 
> > On Mon, Apr 29, 2024 at 07:55:22AM -0600, Zeng, Oak wrote:
> > > Hi Matt
> > >
> > > > -----Original Message-----
> > > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > > Matthew Brost
> > > > Sent: Friday, April 26, 2024 7:33 PM
> > > > To: intel-xe@lists.freedesktop.org
> > > > Cc: Brost, Matthew <matthew.brost@intel.com>; Thomas Hellström
> > > > <thomas.hellstrom@linux.intel.com>; stable@vger.kernel.org
> > > > Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
> > > >
> > > > To be secure, when a userptr is invalidated the pages should be dma
> > > > unmapped ensuring the device can no longer touch the invalidated pages.
> > > >
> > > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
> > GPUs")
> > > > Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user
> > > > pages")
> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > Cc: stable@vger.kernel.org # 6.8
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_vm.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > > index dfd31b346021..964a5b4d47d8 100644
> > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct
> > > > mmu_interval_notifier *mni,
> > > >  		XE_WARN_ON(err);
> > > >  	}
> > > >
> > > > +	if (userptr->sg)
> > > > +		xe_hmm_userptr_free_sg(uvma);
> > >
> > > Just some thoughts here. I think when we introduce system allocator,
> > above should be made conditional. We should dma unmap userptr only for
> > normal userptr but not for userptr created for system allocator (fault usrptr
> > in the system allocator series). Because for system allocator the dma-
> > unmapping would be part of the garbage collector and vma destroy process.
> > Right?
> > >
> > 
> > I don't think it should be conditional. In any case when a CPU address
> > is invalidated we need to ensure the dma mapping (IOMMU mapping) is
> > also invalid to ensure no path to the old (invalidate) pages exists.
> 
> I understand for both normal userptr and fault userptr we need to dma unmap.
> 
> I was saying, for fault userptr, the dma unmap would be done in the garbage collector codes (we destroy fault userptr vma there and dma unmap along with vam destroy), so we don't need dma unmap in your above codes. It would something like this:
> 

I understand what you are suggesting, but no this is always needed.

> If (userptr && not fault userptr)
> 	Dma-unmap sg
> 

With what you suggest, there is a window between the MMU notifier
completing and garbage collector running in which the dma-mapping is
valid. This is not allowed per the security model. Thus we need always
invalidate dma-addresses in the notifier.

Matt

> If (fault userptr)
> 	Trigger garbage collector - this will deal with dma-unmap
> 
> 
> Oak 
> 
> 
> > This is an extra security that must be enforced. With removing the dma
> > mapping, in theory rouge accesses from the GPU could still access the
> > old pages.
> > 
> > Matt
> > 
> > > Oak
> > >
> > > > +
> > > >  	trace_xe_vma_userptr_invalidate_complete(vma);
> > > >
> > > >  	return true;
> > > > --
> > > > 2.34.1
> > >