From mboxrd@z Thu Jan 1 00:00:00 1970 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 Message-ID: <1452622595.9674.19.camel@redhat.com> From: Alex Williamson 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> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] vfio/common: Check iova with limit not with size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pierre Morel Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, peter.maydell@linaro.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. >=20 > But the calculation uses the size of the region instead of > using the region's limit (size - 1). >=20 > 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(). >=20 > 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. >=20 > Signed-off-by: Pierre Morel > --- >=20 > Changes from v2: > =C2=A0=C2=A0=C2=A0=C2=A0- all, just ignore v2, sorry about this, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0this is build after v1 >=20 > Changes from v1: > =C2=A0=C2=A0=C2=A0=C2=A0- adjust the tests by knowing we already substr= acted one to end. >=20 > =C2=A0hw/vfio/common.c |=C2=A0=C2=A0=C2=A014 +++++++------- > =C2=A01 files changed, 7 insertions(+), 7 deletions(-) >=20 > 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, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (int128_ge(int128_make64(iova), llend)= ) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > -=C2=A0=C2=A0=C2=A0=C2=A0end =3D int128_get64(llend); > +=C2=A0=C2=A0=C2=A0=C2=A0end =3D int128_get64(int128_sub(llend, int128_= one())); > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0if ((iova < container->min_iova) || ((end - 1)= > container- > >max_iova)) { > +=C2=A0=C2=A0=C2=A0=C2=A0if ((iova < container->min_iova) || (end=C2=A0= =C2=A0> container- > >max_iova)) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0error_report("vfi= o: IOMMU container %p can't map guest IOVA > region" > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0" 0x%"HWADDR_= PRIx"..0x%"HWADDR_PRIx, > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0container, iova, en= d - 1); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0container, iova, en= d); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D -EFAULT; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto fail; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > @@ -363,7 +363,7 @@ static void > vfio_listener_region_add(MemoryListener *listener, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (memory_region_is_iommu(section->mr)) = { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0VFIOGuestIOMMU *g= iommu; > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_vfio_listener_re= gion_add_iommu(iova, end - 1); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_vfio_listener_re= gion_add_iommu(iova, end); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* FIXME: We= should do some checking to see if the > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* capabilit= ies of the host VFIO IOMMU are adequate to model > @@ -394,13 +394,13 @@ static void > vfio_listener_region_add(MemoryListener *listener, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0section->offset_within_region + > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0(iova - section->offset_within_address_space); > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0trace_vfio_listener_region_add_ram(iova, end -= 1, vaddr); > +=C2=A0=C2=A0=C2=A0=C2=A0trace_vfio_listener_region_add_ram(iova, end, = vaddr); > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0ret =3D vfio_dma_map(container, iova, end - io= va, vaddr, section- > >readonly); > +=C2=A0=C2=A0=C2=A0=C2=A0ret =3D vfio_dma_map(container, iova, end - io= va + 1, vaddr, > section->readonly); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ret) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0error_report("vfi= o_dma_map(%p, 0x%"HWADDR_PRIx", " > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"0x%"HWADDR_P= RIx", %p) =3D %d (%m)", > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0container, iova, en= d - iova, vaddr, ret); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0container, iova, en= d - iova + 1, vaddr, ret); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto fail; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > =C2=A0 Hmm, did we just push the overflow from one place to another? =C2=A0If we= 're mapping a full region of size=C2=A0int128_2_64() starting at iova zero, t= hen this becomes (0xffff_ffff_ffff_ffff - 0 + 1) =3D 0. =C2=A0So I think we n= eed 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, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0MemoryRegionSection *section) =C2=A0{ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0VFIOContainer *container =3D container_of(l= istener, VFIOContainer, listener); -=C2=A0=C2=A0=C2=A0=C2=A0hwaddr iova, end; +=C2=A0=C2=A0=C2=A0=C2=A0hwaddr iova, end, size; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Int128 llend; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0void *vaddr; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret; @@ -348,7 +348,9 @@ static void vfio_listener_region_add(MemoryListener *= listener, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (int128_ge(int128_make64(iova), llend)) = { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0end =3D int128_get64(int128_sub(llend, int1= 28_one())); +=C2=A0=C2=A0=C2=A0=C2=A0size =3D int128_get64(int128_sub(llend, int128_m= ake64(iova))); =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ((iova < container->min_iova) || (end=C2= =A0=C2=A0> container->max_iova)) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0error_report("vfio:= IOMMU container %p can't map guest IOVA region" @@ -396,11 +398,11 @@ static void vfio_listener_region_add(MemoryListener= *listener, =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_vfio_listener_region_add_ram(iova, en= d, vaddr); =C2=A0 -=C2=A0=C2=A0=C2=A0=C2=A0ret =3D vfio_dma_map(container, iova, end - iova= + 1, vaddr, section->readonly); +=C2=A0=C2=A0=C2=A0=C2=A0ret =3D vfio_dma_map(container, iova, size, vadd= r, section->readonly); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ret) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0error_report("vfio_= dma_map(%p, 0x%"HWADDR_PRIx", " =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"0x%"HWADDR_PRIx= ", %p) =3D %d (%m)", -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0container, iova, en= d - iova + 1, vaddr, ret); +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0container, iova, si= ze, vaddr, ret); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto fail; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} =C2=A0 Does that still solve your scenario? =C2=A0Perhaps vfio-iommu-type1 shoul= d 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. =C2=A0Seems like we could do it with two calls to vfio_dma_map if we really wanted to. Thanks, Alex