[PATCH] amd_iommu: Fix leak in free_pagetable()

[PATCH] amd_iommu: Fix leak in free_pagetable()

[Alex Williamson]

AMD IOMMU initializes domains with a 3 level page table by default
and will dynamically size it up to a 6 level page table.  Sadly,
free_pagetable() ignores this feature and statically frees as if
it's a 3 level page table.  Recurse through all the levels to
free everything.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: stable@vger.kernel.org
---

This is obviously a version rewritten to be recursive.  I'll also
post a flat version, take your pick.

 drivers/iommu/amd_iommu.c |   36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 565c745..5496025 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1906,32 +1906,26 @@ static void domain_id_free(int id)
 	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
 }
 
-static void free_pagetable(struct protection_domain *domain)
+static void free_pagetable_level(int level, int max_level, u64 *pt)
 {
-	int i, j;
-	u64 *p1, *p2, *p3;
+	if (level < max_level) {
+		int i;
+		for (i = 0; i < 512; ++i) {
+			if (IOMMU_PTE_PRESENT(pt[i]))
+				free_pagetable_level(level + 1, max_level,
+						     IOMMU_PTE_PAGE(pt[i]));
+		}
+	}
 
-	p1 = domain->pt_root;
+	free_page((unsigned long)pt);
+}
 
-	if (!p1)
+static void free_pagetable(struct protection_domain *domain)
+{
+	if (!domain->pt_root)
 		return;
 
-	for (i = 0; i < 512; ++i) {
-		if (!IOMMU_PTE_PRESENT(p1[i]))
-			continue;
-
-		p2 = IOMMU_PTE_PAGE(p1[i]);
-		for (j = 0; j < 512; ++j) {
-			if (!IOMMU_PTE_PRESENT(p2[j]))
-				continue;
-			p3 = IOMMU_PTE_PAGE(p2[j]);
-			free_page((unsigned long)p3);
-		}
-
-		free_page((unsigned long)p2);
-	}
-
-	free_page((unsigned long)p1);
+	free_pagetable_level(PAGE_MODE_1_LEVEL, domain->mode, domain->pt_root);
 
 	domain->pt_root = NULL;
 }

[PATCH] amd_iommu: Fix leak in free_pagetable()

[Alex Williamson]

AMD IOMMU initializes domains with a 3 level page table by default
and will dynamically size it up to a 6 level page table.  Sadly,
free_pagetable() ignores this feature and statically frees as if
it's a 3 level page table.  Add support for the extra levels.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: stable@vger.kernel.org
---

Here's the flat version.  It's not terrible, but checkpatch whines
about the level of nesting.

 drivers/iommu/amd_iommu.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 565c745..95da421 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1908,23 +1908,47 @@ static void domain_id_free(int id)
 
 static void free_pagetable(struct protection_domain *domain)
 {
-	int i, j;
-	u64 *p1, *p2, *p3;
+	int i, j, k, l, m, depth = domain->mode;
+	u64 *p1, *p2, *p3, *p4, *p5, *p6;
 
 	p1 = domain->pt_root;
 
 	if (!p1)
 		return;
 
-	for (i = 0; i < 512; ++i) {
+	for (i = 0; depth > 1 && i < 512; ++i) {
 		if (!IOMMU_PTE_PRESENT(p1[i]))
 			continue;
 
 		p2 = IOMMU_PTE_PAGE(p1[i]);
-		for (j = 0; j < 512; ++j) {
+		for (j = 0; depth > 2 && j < 512; ++j) {
 			if (!IOMMU_PTE_PRESENT(p2[j]))
 				continue;
+
 			p3 = IOMMU_PTE_PAGE(p2[j]);
+			for (k = 0; depth > 3 && k < 512; ++k) {
+				if (!IOMMU_PTE_PRESENT(p3[k]))
+					continue;
+
+				p4 = IOMMU_PTE_PAGE(p3[k]);
+				for (l = 0; depth > 4 && l < 512; ++l) {
+					if (!IOMMU_PTE_PRESENT(p4[l]))
+						continue;
+
+					p5 = IOMMU_PTE_PAGE(p4[l]);
+					for (m = 0; depth > 5 && m < 512; ++m) {
+						if (!IOMMU_PTE_PRESENT(p5[m]))
+							continue;
+						p6 = IOMMU_PTE_PAGE(p5[m]);
+						free_page((unsigned long)p6);
+					}
+
+					free_page((unsigned long)p5);
+				}
+
+				free_page((unsigned long)p4);
+			}
+
 			free_page((unsigned long)p3);
 		}
 

Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

[Joerg Roedel]

On Mon, Jun 17, 2013 at 07:52:14PM -0600, Alex Williamson wrote:
>  static void free_pagetable(struct protection_domain *domain)
>  {
> -	int i, j;
> -	u64 *p1, *p2, *p3;
> +	int i, j, k, l, m, depth = domain->mode;
> +	u64 *p1, *p2, *p3, *p4, *p5, *p6;
>  
>  	p1 = domain->pt_root;
>  
>  	if (!p1)
>  		return;
>  
> -	for (i = 0; i < 512; ++i) {
> +	for (i = 0; depth > 1 && i < 512; ++i) {
>  		if (!IOMMU_PTE_PRESENT(p1[i]))
>  			continue;
>  
>  		p2 = IOMMU_PTE_PAGE(p1[i]);
> -		for (j = 0; j < 512; ++j) {
> +		for (j = 0; depth > 2 && j < 512; ++j) {
>  			if (!IOMMU_PTE_PRESENT(p2[j]))
>  				continue;
> +
>  			p3 = IOMMU_PTE_PAGE(p2[j]);
> +			for (k = 0; depth > 3 && k < 512; ++k) {
> +				if (!IOMMU_PTE_PRESENT(p3[k]))
> +					continue;
> +
> +				p4 = IOMMU_PTE_PAGE(p3[k]);
> +				for (l = 0; depth > 4 && l < 512; ++l) {
> +					if (!IOMMU_PTE_PRESENT(p4[l]))
> +						continue;
> +
> +					p5 = IOMMU_PTE_PAGE(p4[l]);
> +					for (m = 0; depth > 5 && m < 512; ++m) {
> +						if (!IOMMU_PTE_PRESENT(p5[m]))
> +							continue;
> +						p6 = IOMMU_PTE_PAGE(p5[m]);
> +						free_page((unsigned long)p6);
> +					}
> +
> +					free_page((unsigned long)p5);
> +				}
> +
> +				free_page((unsigned long)p4);
> +			}
> +
>  			free_page((unsigned long)p3);
>  		}

Hmm, actually a recursive version would make more sense here. But since
recursion is a bad idea in the kernel, how about this approach instead:

>From d500d538ad1370679d05667663dcaf8603d529db Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joro@8bytes.org>
Date: Thu, 20 Jun 2013 20:22:58 +0200
Subject: [PATCH] iommu/amd: Fix memory leak in free_pagetable

The IOMMU pagetables can have up to 3 levels, but the code
in free_pagetable() only releases the first 3 levels. Fix
this leak by releasing all levels.

Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 drivers/iommu/amd_iommu.c |   73 ++++++++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 21d02b0..5cde682 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1893,34 +1893,59 @@ static void domain_id_free(int id)
 	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
 }
 
+#define DEFINE_FREE_PT_FN(LVL, FN)				\
+static void free_pt_##LVL (unsigned long __pt)			\
+{								\
+	unsigned long p;					\
+	u64 *pt;						\
+	int i;							\
+								\
+	pt = (u64 *)__pt;					\
+								\
+	for (i = 0; i < 512; ++i) {				\
+		if (!IOMMU_PTE_PRESENT(pt[i]))			\
+			continue;				\
+								\
+		p = (unsigned long)IOMMU_PTE_PAGE(pt[i]);	\
+		FN(p);						\
+	}							\
+	free_page((unsigned long)pt);				\
+}
+
+DEFINE_FREE_PT_FN(l2, free_page)
+DEFINE_FREE_PT_FN(l3, free_pt_l2)
+DEFINE_FREE_PT_FN(l4, free_pt_l3)
+DEFINE_FREE_PT_FN(l5, free_pt_l4)
+DEFINE_FREE_PT_FN(l6, free_pt_l5)
+
 static void free_pagetable(struct protection_domain *domain)
 {
-	int i, j;
-	u64 *p1, *p2, *p3;
-
-	p1 = domain->pt_root;
-
-	if (!p1)
-		return;
-
-	for (i = 0; i < 512; ++i) {
-		if (!IOMMU_PTE_PRESENT(p1[i]))
-			continue;
+	unsigned long root = (unsigned long)domain->pt_root;
 
-		p2 = IOMMU_PTE_PAGE(p1[i]);
-		for (j = 0; j < 512; ++j) {
-			if (!IOMMU_PTE_PRESENT(p2[j]))
-				continue;
-			p3 = IOMMU_PTE_PAGE(p2[j]);
-			free_page((unsigned long)p3);
-		}
-
-		free_page((unsigned long)p2);
+	switch (domain->mode) {
+	case PAGE_MODE_NONE:
+		break;
+	case PAGE_MODE_1_LEVEL:
+		free_page(root);
+		break;
+	case PAGE_MODE_2_LEVEL:
+		free_pt_l2(root);
+		break;
+	case PAGE_MODE_3_LEVEL:
+		free_pt_l3(root);
+		break;
+	case PAGE_MODE_4_LEVEL:
+		free_pt_l4(root);
+		break;
+	case PAGE_MODE_5_LEVEL:
+		free_pt_l5(root);
+		break;
+	case PAGE_MODE_6_LEVEL:
+		free_pt_l6(root);
+		break;
+	default:
+		BUG();
 	}
-
-	free_page((unsigned long)p1);
-
-	domain->pt_root = NULL;
 }
 
 static void free_gcr3_tbl_level1(u64 *tbl)
-- 
1.7.9.5

Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

[Alex Williamson]

On Thu, 2013-06-20 at 20:28 +0200, Joerg Roedel wrote:
> On Mon, Jun 17, 2013 at 07:52:14PM -0600, Alex Williamson wrote:
> >  static void free_pagetable(struct protection_domain *domain)
> >  {
> > -	int i, j;
> > -	u64 *p1, *p2, *p3;
> > +	int i, j, k, l, m, depth = domain->mode;
> > +	u64 *p1, *p2, *p3, *p4, *p5, *p6;
> >  
> >  	p1 = domain->pt_root;
> >  
> >  	if (!p1)
> >  		return;
> >  
> > -	for (i = 0; i < 512; ++i) {
> > +	for (i = 0; depth > 1 && i < 512; ++i) {
> >  		if (!IOMMU_PTE_PRESENT(p1[i]))
> >  			continue;
> >  
> >  		p2 = IOMMU_PTE_PAGE(p1[i]);
> > -		for (j = 0; j < 512; ++j) {
> > +		for (j = 0; depth > 2 && j < 512; ++j) {
> >  			if (!IOMMU_PTE_PRESENT(p2[j]))
> >  				continue;
> > +
> >  			p3 = IOMMU_PTE_PAGE(p2[j]);
> > +			for (k = 0; depth > 3 && k < 512; ++k) {
> > +				if (!IOMMU_PTE_PRESENT(p3[k]))
> > +					continue;
> > +
> > +				p4 = IOMMU_PTE_PAGE(p3[k]);
> > +				for (l = 0; depth > 4 && l < 512; ++l) {
> > +					if (!IOMMU_PTE_PRESENT(p4[l]))
> > +						continue;
> > +
> > +					p5 = IOMMU_PTE_PAGE(p4[l]);
> > +					for (m = 0; depth > 5 && m < 512; ++m) {
> > +						if (!IOMMU_PTE_PRESENT(p5[m]))
> > +							continue;
> > +						p6 = IOMMU_PTE_PAGE(p5[m]);
> > +						free_page((unsigned long)p6);
> > +					}
> > +
> > +					free_page((unsigned long)p5);
> > +				}
> > +
> > +				free_page((unsigned long)p4);
> > +			}
> > +
> >  			free_page((unsigned long)p3);
> >  		}
> 
> Hmm, actually a recursive version would make more sense here. But since
> recursion is a bad idea in the kernel, how about this approach instead:

It's a fixed maximum depth of recursion though, is it really that taboo?

> From d500d538ad1370679d05667663dcaf8603d529db Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joro@8bytes.org>
> Date: Thu, 20 Jun 2013 20:22:58 +0200
> Subject: [PATCH] iommu/amd: Fix memory leak in free_pagetable
> 
> The IOMMU pagetables can have up to 3 levels, but the code

s/3/6/

> in free_pagetable() only releases the first 3 levels. Fix
> this leak by releasing all levels.
> 
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Joerg Roedel <joro@8bytes.org>
> ---
>  drivers/iommu/amd_iommu.c |   73 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 21d02b0..5cde682 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1893,34 +1893,59 @@ static void domain_id_free(int id)
>  	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
>  }
>  
> +#define DEFINE_FREE_PT_FN(LVL, FN)				\
> +static void free_pt_##LVL (unsigned long __pt)			\
> +{								\
> +	unsigned long p;					\
> +	u64 *pt;						\
> +	int i;							\
> +								\
> +	pt = (u64 *)__pt;					\
> +								\
> +	for (i = 0; i < 512; ++i) {				\
> +		if (!IOMMU_PTE_PRESENT(pt[i]))			\
> +			continue;				\
> +								\
> +		p = (unsigned long)IOMMU_PTE_PAGE(pt[i]);	\
> +		FN(p);						\
> +	}							\
> +	free_page((unsigned long)pt);				\
> +}
> +
> +DEFINE_FREE_PT_FN(l2, free_page)
> +DEFINE_FREE_PT_FN(l3, free_pt_l2)
> +DEFINE_FREE_PT_FN(l4, free_pt_l3)
> +DEFINE_FREE_PT_FN(l5, free_pt_l4)
> +DEFINE_FREE_PT_FN(l6, free_pt_l5)
> +
>  static void free_pagetable(struct protection_domain *domain)
>  {
> -	int i, j;
> -	u64 *p1, *p2, *p3;
> -
> -	p1 = domain->pt_root;
> -
> -	if (!p1)
> -		return;
> -
> -	for (i = 0; i < 512; ++i) {
> -		if (!IOMMU_PTE_PRESENT(p1[i]))
> -			continue;
> +	unsigned long root = (unsigned long)domain->pt_root;
>  
> -		p2 = IOMMU_PTE_PAGE(p1[i]);
> -		for (j = 0; j < 512; ++j) {
> -			if (!IOMMU_PTE_PRESENT(p2[j]))
> -				continue;
> -			p3 = IOMMU_PTE_PAGE(p2[j]);
> -			free_page((unsigned long)p3);
> -		}
> -
> -		free_page((unsigned long)p2);
> +	switch (domain->mode) {
> +	case PAGE_MODE_NONE:
> +		break;
> +	case PAGE_MODE_1_LEVEL:
> +		free_page(root);
> +		break;
> +	case PAGE_MODE_2_LEVEL:
> +		free_pt_l2(root);
> +		break;
> +	case PAGE_MODE_3_LEVEL:
> +		free_pt_l3(root);
> +		break;
> +	case PAGE_MODE_4_LEVEL:
> +		free_pt_l4(root);
> +		break;
> +	case PAGE_MODE_5_LEVEL:
> +		free_pt_l5(root);
> +		break;
> +	case PAGE_MODE_6_LEVEL:
> +		free_pt_l6(root);
> +		break;
> +	default:
> +		BUG();
>  	}
> -
> -	free_page((unsigned long)p1);
> -
> -	domain->pt_root = NULL;
>  }
>  
>  static void free_gcr3_tbl_level1(u64 *tbl)

Seems like it should do the right thing

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

[Joerg Roedel]

On Thu, Jun 20, 2013 at 01:08:00PM -0600, Alex Williamson wrote:
> On Thu, 2013-06-20 at 20:28 +0200, Joerg Roedel wrote:
> > Hmm, actually a recursive version would make more sense here. But since
> > recursion is a bad idea in the kernel, how about this approach instead:
> 
> It's a fixed maximum depth of recursion though, is it really that taboo?

Well, a recursive version would be better here because it has lower icache
impact than my patch. But the risk is if there is a bug in recursive
code and it goes on forever it will overwrite random kernel memory, with
undefined, potentially very bad, results (like data corruption, killed
filesystems and so on).

So in my opinion it is better to not take that risk whenever possible.

> > The IOMMU pagetables can have up to 3 levels, but the code
> 
> s/3/6/

Right, thanks.

> Seems like it should do the right thing

Ok, I also gave it some testing (started a vm, assigned and de-assigned
a device), so it should work as expected.

> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Thanks,

	Joerg

Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

[Alex Williamson]

On Thu, 2013-06-20 at 21:26 +0200, Joerg Roedel wrote:
> On Thu, Jun 20, 2013 at 01:08:00PM -0600, Alex Williamson wrote:
> > On Thu, 2013-06-20 at 20:28 +0200, Joerg Roedel wrote:
> > > Hmm, actually a recursive version would make more sense here. But since
> > > recursion is a bad idea in the kernel, how about this approach instead:
> > 
> > It's a fixed maximum depth of recursion though, is it really that taboo?
> 
> Well, a recursive version would be better here because it has lower icache
> impact than my patch. But the risk is if there is a bug in recursive
> code and it goes on forever it will overwrite random kernel memory, with
> undefined, potentially very bad, results (like data corruption, killed
> filesystems and so on).
> 
> So in my opinion it is better to not take that risk whenever possible.

But that's true of a bug in any kernel code.  I think the only danger
unique to recursion is using too much stack space, but I doubt that's
really an issue for a tiny function with a fixed depth like this.  In
case you didn't notice, I did send a recursive version along with the
flat version, but I stupidly used the same subject for both.  It's a
little less code and a tiny bit easier to understand than the macro
version.  Up to you though.  Thanks,

Alex

> > > The IOMMU pagetables can have up to 3 levels, but the code
> > 
> > s/3/6/
> 
> Right, thanks.
> 
> > Seems like it should do the right thing
> 
> Ok, I also gave it some testing (started a vm, assigned and de-assigned
> a device), so it should work as expected.
> 
> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Thanks,
> 
> 	Joerg
> 
> 

Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

[Joerg Roedel]

On Thu, Jun 20, 2013 at 01:46:48PM -0600, Alex Williamson wrote:
> But that's true of a bug in any kernel code.  I think the only danger
> unique to recursion is using too much stack space, but I doubt that's
> really an issue for a tiny function with a fixed depth like this.  In
> case you didn't notice, I did send a recursive version along with the
> flat version, but I stupidly used the same subject for both.  It's a
> little less code and a tiny bit easier to understand than the macro
> version.  Up to you though.

Well, the limited kernel stack is exactly the problem for an
unterminating recursion. The size of the stack-footprint does also not
really matter in this situation, the code will overwrite random kernel
memory when the stack overflows. And that memory could potentially
contain data that is about to be written to disk and destroy file
systems.


	Joerg