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

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

[Alex Williamson]

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 <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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);

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

[Alex Williamson]

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@vger.kernel.org
Signed-off-by: Alex Williamson <alex.williamson@redhat.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);

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

[Marcelo Tosatti]

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 <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Marcelo Tosatti]

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