Re: [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in non-coherent domains

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 16 May 2024 08:34:20 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Zhao, Yan Y <yan.y.zhao@intel.com>
> > Sent: Monday, May 13, 2024 3:11 PM
> > On Fri, May 10, 2024 at 10:57:28AM -0600, Alex Williamson wrote:  
> > > On Fri, 10 May 2024 18:31:13 +0800
> > > Yan Zhao <yan.y.zhao@intel.com> wrote:
> > >  
> > > > On Thu, May 09, 2024 at 12:10:49PM -0600, Alex Williamson wrote:  
> > > > > On Tue,  7 May 2024 14:21:38 +0800
> > > > > Yan Zhao <yan.y.zhao@intel.com> wrote:  
> > > > > > @@ -1847,6 +1891,9 @@ static void vfio_test_domain_fgsp(struct  
> > vfio_domain *domain, struct list_head *  
> > > > > >  		break;
> > > > > >  	}
> > > > > >
> > > > > > +	if (!domain->enforce_cache_coherency)
> > > > > > +		arch_clean_nonsnoop_dma(page_to_phys(pages),  
> > PAGE_SIZE * 2);  
> > > > > > +  
> > > > >
> > > > > Seems like this use case isn't subject to the unmap aspect since these
> > > > > are kernel allocated and freed pages rather than userspace pages.
> > > > > There's not an "ongoing use of the page" concern.
> > > > >
> > > > > The window of opportunity for a device to discover and exploit the
> > > > > mapping side issue appears almost impossibly small.
> > > > >  
> > > > The concern is for a malicious device attempting DMAs automatically.
> > > > Do you think this concern is valid?
> > > > As there're only extra flushes for 4 pages, what about keeping it for safety?  
> > >
> > > Userspace doesn't know anything about these mappings, so to exploit
> > > them the device would somehow need to discover and interact with the
> > > mapping in the split second that the mapping exists, without exposing
> > > itself with mapping faults at the IOMMU.  
> 
> Userspace could guess the attacking ranges based on code, e.g. currently
> the code just tries to use the 1st available IOVA region which likely starts
> at address 0.
> 
> and mapping faults don't stop the attack. Just some after-the-fact hint
> revealing the possibility of being attacked. 😊

As below, the gap is infinitesimally small, but not zero, and I don't
mind closing it entirely.

> > >
> > > I don't mind keeping the flush before map so that infinitesimal gap
> > > where previous data in physical memory exposed to the device is closed,
> > > but I have a much harder time seeing that the flush on unmap to
> > > synchronize physical memory is required.
> > >
> > > For example, the potential KSM use case doesn't exist since the pages
> > > are not owned by the user.  Any subsequent use of the pages would be
> > > subject to the same condition we assumed after allocation, where the
> > > physical data may be inconsistent with the cached data.  It's easy to  
> 
> physical data can be different from the cached one at any time. In normal
> case the cache line is marked as dirty and the CPU cache protocol
> guarantees coherency between cache/memory.
> 
> here we talked about a situation which a malicious user uses non-coherent
> DMA to bypass CPU and makes memory/cache inconsistent when the
> CPU still considers the memory copy is up-to-date (e.g. cacheline is in
> exclusive or shared state). In this case multiple reads from the next-user
> may get different values from cache or memory depending on when the
> cacheline is invalidated.
> 
> So it's really about a bad inconsistency state which can be recovered only
> by invalidating the cacheline (so memory data is up-to-date) or doing
> a WB-type store (to mark memory copy out-of-date) before the next-use.

Ok, so the initial state may be that the page is zero'd in cache, but
the cacheline is dirty and is therefore the source of truth for all
coherent operations.  In the case where a device has non-coherently
modified physical memory, the coherent results are indeterminate, the
processor could see a value from cache or physical memory.  So these
are in fact different scenarios.

> > > flush 2 pages, but I think it obscures the function of the flush if we
> > > can't articulate the value in this case.  
> 
> btw KSM is one example. Jason mentioned in earlier discussion that not all
> free pages are zero-ed before the next use then it'd always good to
> conservatively prevent any potential inconsistent state leaked back to
> the kernel. Though I'm not sure what'd be a real usage in which the next
> user will directly use then uninitialized content w/o doing any meaningful
> writes (which once done then will stop the attacking window)...

Yes, exactly.  Zero'ing the page would obviously reestablish the
coherency, but the page could be reallocated without being zero'd and as
you describe the owner of that page could then get inconsistent
results.  It doesn't fit any use case that I can think of that next
user only cares that the contents of the page are consistent without
writing a specific value, but sure, let's not be the source of that
obscure bug ;)  Thanks,

Alex