From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52827) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGtRL-00055E-AN for qemu-devel@nongnu.org; Wed, 06 Jan 2016 14:08:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGtRG-0004CX-A0 for qemu-devel@nongnu.org; Wed, 06 Jan 2016 14:08:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40472) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGtRG-0004CR-2s for qemu-devel@nongnu.org; Wed, 06 Jan 2016 14:08:06 -0500 Message-ID: <1452107285.29599.127.camel@redhat.com> From: Alex Williamson Date: Wed, 06 Jan 2016 12:08:05 -0700 In-Reply-To: <1452009820-24968-1-git-send-email-pmorel@linux.vnet.ibm.com> References: <1452009820-24968-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 v2] 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-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(). >=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 > --- > =C2=A0hw/vfio/common.c |=C2=A0=C2=A0=C2=A015 ++++++++++----- > =C2=A01 files changed, 10 insertions(+), 5 deletions(-) >=20 > 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(MemoryListen= er *listener, > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iova =3D TARGET_PAGE_ALIGN(section->offse= t_within_address_space); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0llend =3D int128_make64(section->offset_w= ithin_address_space); > -=C2=A0=C2=A0=C2=A0=C2=A0llend =3D int128_add(llend, section->size); > + > +=C2=A0=C2=A0=C2=A0=C2=A0if (int128_ge(llend, int128_2_64())) { We've just set llend using int128_make64, so this is guaranteed false. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0llend =3D int128_add(l= lend, int128_sub(section->size, int128_one())); > +=C2=A0=C2=A0=C2=A0=C2=A0} else { > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0llend =3D int128_add(l= lend, section->size); > +=C2=A0=C2=A0=C2=A0=C2=A0} So the above changed nothing. > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0llend =3D int128_and(llend, int128_exts64= (TARGET_PAGE_MASK)); > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0if (int128_ge(int128_make64(iova), llend)) { > +=C2=A0=C2=A0=C2=A0=C2=A0if (int128_gt(int128_make64(iova), llend)) { And this allows zero sized regions through. > =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(llend); > =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 > con= tainer->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); 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. =C2=A0A mapping of size 4k at offset 0 should work, but this change would cause it to fail. > =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 +368,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 I think maybe you want to set end using: end =3D int128_get64(int128_sub(llend,=C2=A0int128_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. =C2=A0Thanks, Alex