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

[Alex Williamson]

On Tue, 2016-01-12 at 16:11 +0100, Pierre Morel wrote:
> In vfio_listener_region_add(), we try to validate that the region is
> not
> zero sized and hasn't overflowed the addresses space.
> 
> But the calculation uses the size of the region instead of
> using the region's limit (size - 1).
> 
> 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>
> ---
> 
> Changes from v2:
>     - all, just ignore v2, sorry about this,
>       this is build after v1
> 
> Changes from v1:
>     - adjust the tests by knowing we already substracted one to end.
> 
>  hw/vfio/common.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6797208..a5f6643 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -348,12 +348,12 @@ static void
> vfio_listener_region_add(MemoryListener *listener,
>      if (int128_ge(int128_make64(iova), llend)) {
>          return;
>      }
> -    end = int128_get64(llend);
> +    end = int128_get64(int128_sub(llend, int128_one()));
>  
> -    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);
>          ret = -EFAULT;
>          goto fail;
>      }
> @@ -363,7 +363,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
> @@ -394,13 +394,13 @@ static void
> vfio_listener_region_add(MemoryListener *listener,
>              section->offset_within_region +
>              (iova - section->offset_within_address_space);
>  
> -    trace_vfio_listener_region_add_ram(iova, end - 1, vaddr);
> +    trace_vfio_listener_region_add_ram(iova, end, vaddr);
>  
> -    ret = vfio_dma_map(container, iova, end - iova, vaddr, section-
> >readonly);
> +    ret = vfio_dma_map(container, iova, end - iova + 1, vaddr,
> section->readonly);
>      if (ret) {
>          error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
>                       "0x%"HWADDR_PRIx", %p) = %d (%m)",
> -                     container, iova, end - iova, vaddr, ret);
> +                     container, iova, end - iova + 1, vaddr, ret);
>          goto fail;
>      }
>  

Hmm, did we just push the overflow from one place to another?  If we're
mapping a full region of size int128_2_64() starting at iova zero, then
this becomes (0xffff_ffff_ffff_ffff - 0 + 1) = 0.  So I think we need
to calculate size with 128bit arithmetic too and let it assert if we
overflow, ie:

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index a5f6643..13ad90b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -321,7 +321,7 @@ static void vfio_listener_region_add(MemoryListener 
*listener,
                                      MemoryRegionSection *section)
 {
     VFIOContainer *container = container_of(listener, VFIOContainer, listener);
-    hwaddr iova, end;
+    hwaddr iova, end, size;
     Int128 llend;
     void *vaddr;
     int ret;
@@ -348,7 +348,9 @@ static void vfio_listener_region_add(MemoryListener 
*listener,
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
+
     end = int128_get64(int128_sub(llend, int128_one()));
+    size = int128_get64(int128_sub(llend, int128_make64(iova)));
 
     if ((iova < container->min_iova) || (end  > container->max_iova)) {
         error_report("vfio: IOMMU container %p can't map guest IOVA region"
@@ -396,11 +398,11 @@ static void vfio_listener_region_add(MemoryListener 
*listener,
 
     trace_vfio_listener_region_add_ram(iova, end, vaddr);
 
-    ret = vfio_dma_map(container, iova, end - iova + 1, vaddr, 
section->readonly);
+    ret = vfio_dma_map(container, iova, size, vaddr, section->readonly);
     if (ret) {
         error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
                      "0x%"HWADDR_PRIx", %p) = %d (%m)",
-                     container, iova, end - iova + 1, vaddr, ret);
+                     container, iova, size, vaddr, ret);
         goto fail;
     }
 

Does that still solve your scenario?  Perhaps vfio-iommu-type1 should
have used first/last rather than start/size for mapping since we seem
to have an off-by-one for mapping a full 64bit space.  Seems like we
could do it with two calls to vfio_dma_map if we really wanted to.
Thanks,

Alex