amd_iommu: Fix leak in free_pagetable()

static void free_pagetable(struct protection_domain *domain) {

(quoted diff) +20 -4

-	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);

From d500d538ad1370679d05667663dcaf8603d529db Mon Sep 17 00:00:00 2001

Quoted 44 lines · Alex Williamson

static void free_pagetable(struct protection_domain *domain) {

(quoted diff) +25 -4

-	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);
+				}
+

drivers/iommu/amd_iommu.c static void domain_id_free(int id) · line 1948

Quoted 5 lines

drivers/iommu/amd_iommu.c +3 -0

+				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 <[email protected]>
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 <[email protected]>

Signed-off-by Joerg Roedel <[email protected]>

---
 drivers/iommu/amd_iommu.c |   73 ++++++++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 24 deletions(-)

drivers/iommu/amd_iommu.c +47 -20

index 21d02b0..5cde682 100644
@@ -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;

Quoted 2 lines · Joerg Roedel

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?

Quoted 1 line

The IOMMU pagetables can have up to 3 levels, but the code

s/3/6/

Seems like it should do the right thing

Reviewed-by Alex Williamson <[email protected]>

Quoted 5 lines · Alex Williamson

Quoted 2 lines · Joerg Roedel

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.

Quoted 3 lines

Quoted 1 line

The IOMMU pagetables can have up to 3 levels, but the code

s/3/6/

Right, thanks.

Quoted 1 line

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.

Quoted 1 line

Reviewed-by Alex Williamson <[email protected]>

Thanks,

Joerg

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.