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

[Bandan Das]

Alex Williamson <address@hidden> writes:

> On Mon, 21 Mar 2016 20:06:32 -0400
> Bandan Das <address@hidden> wrote:
>
>> Alex Williamson <address@hidden> writes:
>> 
>> > On Mon, 21 Mar 2016 18:00:50 -0400
>> > Bandan Das <address@hidden> 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 <address@hidden>
>> >> ---
>> >>  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.

> 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. 

> 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.

> 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.

> likely the system page size.  Thanks,
>
> Alex