Re: [Qemu-devel] [PATCH v2] vfio/common: Check iova with limit not with size

[Pierre Morel <pmorel@linux.vnet.ibm.com>]

On 01/06/2016 08:08 PM, Alex Williamson wrote:
> On Tue, 2016-01-05 at 17:03 +0100, Pierre Morel wrote:
>> In vfio_listener_region_add(), the code makes sure
>> that the offset in the section is lower than the size
>> of the section.
>> But the calculation uses size of the region instead of
>> the region's limit (size - 1).
> We're really just trying to validate that the region is not zero sized
> and hasn't overflowed the addresses space.
>
>> This leads to Int128 overflow when the region has
>> been initialized to UINT64_MAX because in this case
>> memory_region_init() transform the size from UINT64_MAX
>> to int128_2_64().
>>
>> Let's really use the limit by sustracting one to the size
>> and take care to use the limit for functions using limit
>> and size to call functions which need size.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   hw/vfio/common.c |   15 ++++++++++-----
>>   1 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 6797208..fe4962a 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -342,18 +342,23 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>   
>>       iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
>>       llend = int128_make64(section->offset_within_address_space);
>> -    llend = int128_add(llend, section->size);
>> +
>> +    if (int128_ge(llend, int128_2_64())) {
> We've just set llend using int128_make64, so this is guaranteed false.

hum, sorry, indeed.

>
>> +        llend = int128_add(llend, int128_sub(section->size, int128_one()));
>> +    } else {
>> +        llend = int128_add(llend, section->size);
>> +    }
> So the above changed nothing.
>
>>       llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
>>   
>> -    if (int128_ge(int128_make64(iova), llend)) {
>> +    if (int128_gt(int128_make64(iova), llend)) {
> And this allows zero sized regions through.
>
>>           return;
>>       }
>>       end = int128_get64(llend);
>>   
>> -    if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
>> +    if ((iova < container->min_iova) || (end > container->max_iova)) {
>>           error_report("vfio: IOMMU container %p can't map guest IOVA region"
>>                        " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
>> -                     container, iova, end - 1);
>> +                     container, iova, end);
> This looks wrong too, max_iova is set to the last valid iova, for
> instance if the iommu only supported a 4k address space, max_iova would
> be 0xfff.  A mapping of size 4k at offset 0 should work, but this
> change would cause it to fail.
>
>>           ret = -EFAULT;
>>           goto fail;
>>       }
>> @@ -363,7 +368,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>       if (memory_region_is_iommu(section->mr)) {
>>           VFIOGuestIOMMU *giommu;
>>   
>> -        trace_vfio_listener_region_add_iommu(iova, end - 1);
>> +        trace_vfio_listener_region_add_iommu(iova, end);
>>           /*
>>            * FIXME: We should do some checking to see if the
>>            * capabilities of the host VFIO IOMMU are adequate to model
> I think maybe you want to set end using:
>
> end = int128_get64(int128_sub(llend, int128_one()));
>
> Then removing the -1 in other places becomes correct, BUT we need to
> add 1 where we're passing the size, end - iova - > end - iova + 1.
>   Thanks,
>
> Alex
>

You are right, I try again.

Pierre