From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49246) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiBrb-00081g-AY for qemu-devel@nongnu.org; Mon, 21 Mar 2016 22:16:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiBrW-0005ea-9D for qemu-devel@nongnu.org; Mon, 21 Mar 2016 22:16:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiBrW-0005eR-1b for qemu-devel@nongnu.org; Mon, 21 Mar 2016 22:16:02 -0400 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 2BD3446208 for ; Tue, 22 Mar 2016 02:16:01 +0000 (UTC) Date: Mon, 21 Mar 2016 20:16:00 -0600 From: Alex Williamson Message-ID: <20160321201600.7073b441@ul30vt.home> In-Reply-To: References: <20160321163441.29684cbf@t450s.home> <20160321183052.59e87b29@ul30vt.home> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vfio: add check for memory region overflow condition List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bandan Das Cc: qemu-devel On Mon, 21 Mar 2016 21:54:48 -0400 Bandan Das wrote: > Alex Williamson writes: > > > On Mon, 21 Mar 2016 20:06:32 -0400 > > Bandan Das wrote: > > > >> Alex Williamson writes: > >> > >> > On Mon, 21 Mar 2016 18:00:50 -0400 > >> > Bandan Das wrote: > >> > > >> >> vfio_listener_region_add for a iommu mr results in > >> >> an overflow assert since emulated iommu memory region is initialized > >> >> with UINT64_MAX. Add a check just like memory_region_size() > >> >> does. > >> >> > >> >> Signed-off-by: Bandan Das > >> >> --- > >> >> hw/vfio/common.c | 7 ++++++- > >> >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> >> index fb588d8..269244b 100644 > >> >> --- a/hw/vfio/common.c > >> >> +++ b/hw/vfio/common.c > >> >> @@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener, > >> >> if (int128_ge(int128_make64(iova), llend)) { > >> >> return; > >> >> } > >> >> - end = int128_get64(llend); > >> >> + > >> >> + if (int128_eq(llend, int128_2_64())) { > >> >> + end = UINT64_MAX; > >> >> + } else { > >> >> + end = int128_get64(llend); > >> >> + } > >> >> > >> >> if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) { > >> >> error_report("vfio: IOMMU container %p can't map guest IOVA region" > >> > > >> > But now all the calculations where we use end-1 are wrong. See the > >> > discussion with Pierre Morel in the January qemu-devel archives. > >> > There's a solution in there, but I never saw a follow-up from Pierre > >> > with a revised patch. Thanks, > >> > >> I am missing something. When end < UIN64_MAX, end - 1 calculations are valid because > >> the patch doesn't change that behavior. When end is UINT64_MAX, int128_get64() doesn't know how > >> to calculate this value and we are just feeding it manually. The patch is just the opposite > >> of what memory_region_init() did to init the mem region in the first place: > >> mr->size = int128_make64(size); > >> if (size == UINT64_MAX) { > >> mr->size = int128_2_64(); > >> } > >> So, end - 1 is still valid for end = UINT64_MAX, no ? > > > > int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to > > @end is clearing altering the value. If we had a range from zero to > > I thought in128_2_64 is the 128 bit representation of UINT64_MAX. The > if condition in memory_region_init doesn't make sense otherwise. 2^64 cannot be represented with a uint64_t, 2^64 - 1 can: int128_2_64 = 1_0000_0000_0000_0000h UINT64_MAX = ffff_ffff_ffff_ffffh > > int128_2_64() then the size of that region is int128_2_64(). If we > > alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end > > - 1 is off by one versus the case where we use the value directly. > > Ok, you mean something like: > int128_get64(int128_sub(int128_2_64(), int128_make64(1))); for (end - 1) ? > But we still have to deal with (end - iova) when calling vfio_dmap_map(). > int128_get64() will definitely assert for iova = 0. I don't know that that's the most efficient way to handle it, but @end represents a different thing by imposing that -1 and it needs to be handled in the reset of the code. > > You're effectively changing @end to be the last address in the range, > > No, I think I am changing "end" to what we initally started with for size > before converting to 128 bit. Nope, it's the difference between the size of the region and the last address of the region. > > but only in some cases, and not adjusting the remaining code to match. > > Not only that, but the vfio map command is probably going to fail if we > > pass in such an unaligned size since the mapping granularity is > > Trying to map such a large region is wrong anyway, I am still trying > to workout a solution to avoid calling memory_region_init_iommu() > with UINT64_MAX which is what emulated vt-d currently does. Right, the address width of the IOMMU on x86 is typically nowhere near 2^64, so if you take the vfio_dma_map path, you'll surely explode. Does this fix actually fix anything or just move us to the next assert? Thanks, Alex