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

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

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
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.
You're effectively changing @end to be the last address in the range,
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
likely the system page size.  Thanks,

Alex