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

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

On Mon, 21 Mar 2016 21:54:48 -0400
Bandan Das <bsd@redhat.com> wrote:

> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Mon, 21 Mar 2016 20:06:32 -0400
> > Bandan Das <bsd@redhat.com> wrote:
> >  
> >> Alex Williamson <alex.williamson@redhat.com> writes:
> >>   
> >> > On Mon, 21 Mar 2016 18:00:50 -0400
> >> > Bandan Das <bsd@redhat.com> wrote:
> >> >    
> >> >> vfio_listener_region_add for a iommu mr results in
> >> >> an overflow assert since emulated iommu memory region is initialized
> >> >> with UINT64_MAX. Add a check just like memory_region_size()
> >> >> does.
> >> >> 
> >> >> Signed-off-by: Bandan Das <bsd@redhat.com>
> >> >> ---
> >> >>  hw/vfio/common.c | 7 ++++++-
> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> >> index fb588d8..269244b 100644
> >> >> --- a/hw/vfio/common.c
> >> >> +++ b/hw/vfio/common.c
> >> >> @@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
> >> >>      if (int128_ge(int128_make64(iova), llend)) {
> >> >>          return;
> >> >>      }
> >> >> -    end = int128_get64(llend);
> >> >> +
> >> >> +    if (int128_eq(llend, int128_2_64())) {
> >> >> +            end = UINT64_MAX;
> >> >> +    } else {
> >> >> +            end = int128_get64(llend);
> >> >> +    }
> >> >>  
> >> >>      if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
> >> >>          error_report("vfio: IOMMU container %p can't map guest IOVA region"    
> >> >
> >> > But now all the calculations where we use end-1 are wrong.  See the
> >> > discussion with Pierre Morel in the January qemu-devel archives.
> >> > There's a solution in there, but I never saw a follow-up from Pierre
> >> > with a revised patch.  Thanks,    
> >> 
> >> I am missing something. When end < UIN64_MAX, end - 1 calculations are valid because
> >> the patch doesn't change that behavior. When end is UINT64_MAX, int128_get64() doesn't know how
> >> to calculate this value and we are just feeding it manually. The patch is just the opposite
> >> of what memory_region_init() did to init the mem region in the first place:
> >>    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
 
> > 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.

> > 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.
Does this fix actually fix anything or just move us to the next
assert?  Thanks,

Alex