Re: [Qemu-devel] [PATCH] vfio: add check for memory region overflow condition

[Alex Williamson]

On Tue, 22 Mar 2016 14:55:14 -0400
Bandan Das <address@hidden> wrote:

> Alex Williamson <address@hidden> writes:
> ...
> >> >>    mr->size = int128_make64(size);
> >> >>    if (size == UINT64_MAX) {
> >> >>       mr->size = int128_2_64();
> >> >>    }
> >> >> So, end - 1 is still valid for end = UINT64_MAX, no ?    
> >> >
> >> > int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to
> >> > @end is clearing altering the value.  If we had a range from zero to    
> >> 
> >> I thought in128_2_64 is the 128 bit representation of UINT64_MAX. The
> >> if condition in memory_region_init doesn't make sense otherwise.  
> >
> > 2^64 cannot be represented with a uint64_t, 2^64 - 1 can:
> >
> > int128_2_64 = 1_0000_0000_0000_0000h
> > UINT64_MAX  =   ffff_ffff_ffff_ffffh  
> 
> Thanks, understood this part. I still don't understand the if condition
> in memory_region_init however. Unless, that function actually takes the
> last address for the size parameter and in that case, it should be
> UINT64_MAX-1 for a size of UINT64_MAX.

Seems like some sort of compatibility convention since
memory_region_init() only takes a uint64_t as the size.  memory.c
interprets that as "oh, I know you really mean 2^64".
 
> >> > int128_2_64() then the size of that region is int128_2_64().  If we
> >> > alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end
> >> > - 1 is off by one versus the case where we use the value directly.    
> >> 
> >> Ok, you mean something like:
> >> int128_get64(int128_sub(int128_2_64(), int128_make64(1)));  for (end - 1) ?
> >> But we still have to deal with (end - iova) when calling vfio_dmap_map().
> >> int128_get64() will definitely assert for iova = 0.   
> >
> > I don't know that that's the most efficient way to handle it, but @end
> > represents a different thing by imposing that -1 and it needs to be
> > handled in the reset of the code.
> >  
> >> > You're effectively changing @end to be the last address in the range,    
> >> 
> >> No, I think I am changing "end" to what we initally started with for size
> >> before converting to 128 bit.  
> >
> > Nope, it's the difference between the size of the region and the last
> > address of the region.  
> 
> Ok, but note that it's the "size" that actually asserts here since the
> offset is 0. So, we started with a size UINT64_MAX but end with mr->size =
> 128_2_64().

A size of UINT64_MAX doesn't make much sense to me, that would mean the
last address is UINT64_MAX - 1.  A size of 2^64 means the last address
is UINT64_MAX, which seems reasonable.
 
> >> > but only in some cases, and not adjusting the remaining code to match.
> >> > Not only that, but the vfio map command is probably going to fail if we
> >> > pass in such an unaligned size since the mapping granularity is    
> >> 
> >> Trying to map such a large region is wrong anyway, I am still trying
> >> to workout a solution to avoid calling memory_region_init_iommu()
> >> with UINT64_MAX which is what emulated vt-d currently does.  
> >
> > Right, the address width of the IOMMU on x86 is typically nowhere near
> > 2^64, so if you take the vfio_dma_map path, you'll surely explode.  
> 
> And it does. If we fix this assert, then vfio_dma_map() attempts mapping
> this direct mapped address range starting from 0 and prints a 
> warning message; happens for the whole range and goes on for ever.
> The overflow check seemed to me like something we should fix, but now
> I am more confused then ever!

Is the MemoryRegion memory_region_is_iommu() such that you're calling
vfio_dma_map() from vfio_iommu_map_notify()?  If so then we should
probably be using 128bit helpers for doing sanity checking and go ahead
and let something assert if we get to the vfio_dma_map() in
vfio_listener_region_add() with a 2^64 size.  Then if you're taking the
memory_region_is_iommu() path, vfio_dma_map() is going to be called
with translations within that 2^64 bit address space, not mapping the
entire space, right?  Thanks,

Alex