From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH] intel-iommu: Fix off-by-one in pagetable freeing Date: Thu, 23 Jan 2014 14:47:28 -0200 Message-ID: <20140123164728.GA23854@amt.cnet> References: <20131220193250.17903.48255.stgit@bling.home> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20131220193250.17903.48255.stgit-xdHQ/5r00wBBDLzU/O5InQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Alex Williamson Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: iommu@lists.linux-foundation.org On Fri, Dec 20, 2013 at 12:33:43PM -0700, Alex Williamson wrote: > dma_pte_free_level() has an off-by-one error when checking whether a > pte is completely covered by a range. Take for example the case of > attempting to free pfn 0x0 - 0x1ff, ie. 512 entries covering the first > 2M superpage. The level_size() is 0x200 and we test: > > static void dma_pte_free_level(... > ... > > if (!(0 > 0 || 0x1ff < 0 + 0x200)) { > ... > } > > Clearly the 2nd test is true, which means we fail to take the branch > to clear and free the pagetable entry. As a result, we're leaking > pagetables and failing to install new pages over the range. > > This was found with a PCI device assigned to a QEMU guest using > vfio-pci without a VGA device present. The first 1M of guest > address space is mapped with various combinations of 4K pages, but > eventually the range is entirely freed and replaced with a 2M > contiguous mapping. intel-iommu errors out with something like: > > ERROR: DMA PTE for vPFN 0x0 already set (to 5c2b8003 not 849c00083) > > In this case 5c2b8003 is the pointer to the previous leaf page that > was neither freed nor cleared and 849c00083 is the superpage entry > that we're trying to replace it with. > > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: Alex Williamson > --- > drivers/iommu/intel-iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c > index 43b9bfe..59779e1 100644 > --- a/drivers/iommu/intel-iommu.c > +++ b/drivers/iommu/intel-iommu.c > @@ -917,7 +917,7 @@ static void dma_pte_free_level(struct dmar_domain *domain, int level, > > /* If range covers entire pagetable, free it */ > if (!(start_pfn > level_pfn || > - last_pfn < level_pfn + level_size(level))) { > + last_pfn < level_pfn + level_size(level) - 1)) { > dma_clear_pte(pte); > domain_flush_cache(domain, pte, sizeof(*pte)); > free_pgtable_page(level_pte); Reviewed-by: Marcelo Tosatti