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

[Alex Williamson]

On Wed, 2016-01-20 at 16:14 +0100, Pierre Morel wrote:
> 
> On 01/12/2016 07:16 PM, Alex Williamson wrote:
> > 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)));
> 
> here again, if iova is null, since llend is section->size (2^64) ...
> 
> >   
> >       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
> > 
> 
> You are right, every try to solve this will push the overflow
> somewhere 
> else.
> 
> There is just no way to express 2^64 with 64 bits, we have the
> int128() 
> solution,
> but if we solve it here, we fall in the linux ioctl call anyway.
> 
> Intuitively, making two calls do not seem right to me.
> 
> But, what do you think of something like:
> 
> - creating a new VFIO extention
> 
> - and in ioctl(), since we have a flag entry in the 
> vfio_iommu_type1_dma_map,
> may be adding a new flag meaning "map all virtual memory" ?
> or meaning "use first/last" ?
> I think this would break existing code unless we add a new VFIO
> extension.

Backup, is there ever a case where we actually need to map the entire
64bit address space?  This is fairly well impossible on x86.  I'm
pointing out an issue, but I don't know that we need to solve it with
more than an assert since it's never likely to happen.  Thanks,

Alex