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

[Alex Williamson]

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 <address@hidden>
> ---
>  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.

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