From patchwork Tue Jan 12 18:16:35 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 566670 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 03C191402C4 for ; Wed, 13 Jan 2016 05:16:58 +1100 (AEDT) Received: from localhost ([::1]:33587 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ3V1-0002Et-TL for incoming@patchwork.ozlabs.org; Tue, 12 Jan 2016 13:16:55 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33063) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ3Um-0001xS-K7 for qemu-devel@nongnu.org; Tue, 12 Jan 2016 13:16:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJ3Uj-0003jx-DC for qemu-devel@nongnu.org; Tue, 12 Jan 2016 13:16:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47670) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ3Uj-0003jQ-3L for qemu-devel@nongnu.org; Tue, 12 Jan 2016 13:16:37 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id A3021C0AA56E; Tue, 12 Jan 2016 18:16:36 +0000 (UTC) Received: from t450s.home (ovpn-113-128.phx2.redhat.com [10.3.113.128]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0CIGZck007824; Tue, 12 Jan 2016 13:16:36 -0500 Message-ID: <1452622595.9674.19.camel@redhat.com> From: Alex Williamson To: Pierre Morel Date: Tue, 12 Jan 2016 11:16:35 -0700 In-Reply-To: <1452611505-25478-1-git-send-email-pmorel@linux.vnet.ibm.com> References: <1452611505-25478-1-git-send-email-pmorel@linux.vnet.ibm.com> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, peter.maydell@linaro.org Subject: Re: [Qemu-devel] [PATCH v3] vfio/common: Check iova with limit not with size X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 > --- > > 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: 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 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;      }