linux-kernel - Re: [PATCH] intel-iommu: Fix off-by-one in pagetable freeing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 23 Jan 2014 14:47:28 -0200
From:	Marcelo Tosatti <mtosatti@...hat.com>
To:	Alex Williamson <alex.williamson@...hat.com>
Cc:	iommu@...ts.linux-foundation.org, dwmw2@...radead.org,
	ddutile@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] intel-iommu: Fix off-by-one in pagetable freeing

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@...r.kernel.org
> Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> ---
>  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 <mtosatti@...hat.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/