Wrongly computed MTRR range addresses

Question

Wrongly computed MTRR range addresses

wbenny opened this issue 6 years ago · 16 comments

Type of this issue (please specify)

This is a bug in the upstream tree as-is unmodified.
This is a support matter (i.e. your own modified tree)
This is a technical question

In mm.c, function mm_cache_mtrr_ranges

...
	if ((cap >> 8) & 1 && (def >> 10) & 1) {
		/* Read fixed range MTRRs.  */
		for (msr = __readmsr(MSR_MTRRfix64K_00000), offset = 0, base = 0;
		     msr != 0; msr >>= 8, offset += 0x10000, base += offset)
			make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x10000 - 1);

		for (val = MSR_MTRRfix16K_80000, offset = 0; val <= MSR_MTRRfix16K_A0000; ++val)
			for (msr = __readmsr(val), base = 0x80000;
			     msr != 0; msr >>= 8, offset += 0x4000, base += offset)
				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x4000 - 1);

		for (val = MSR_MTRRfix4K_C0000, offset = 0; val <= MSR_MTRRfix4K_F8000; ++val)
			for (msr = __readmsr(val), base = 0xC0000;
			     msr != 0; msr >>= 8, offset += 0x1000, base += offset)
				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x1000 - 1);
	}
...

offset should not be incremented each inner loop - it should be set once (with base, in the initial part of the for expression).

This error creates this pattern:

ksm: CPU 1: ksm_init: MTRR Range: 0x0000000000000000 -> 0x000000000000FFFF fixed: 1 type: 6
ksm: CPU 1: ksm_init: MTRR Range: 0x0000000000010000 -> 0x000000000001FFFF fixed: 1 type: 6
ksm: CPU 1: ksm_init: MTRR Range: 0x0000000000030000 -> 0x000000000003FFFF fixed: 1 type: 6 // should be 0x0000000000020000 -> 0x000000000002FFFF
ksm: CPU 1: ksm_init: MTRR Range: 0x0000000000060000 -> 0x000000000006FFFF fixed: 1 type: 6 // should be 0x0000000000030000 -> 0x000000000003FFFF
ksm: CPU 1: ksm_init: MTRR Range: 0x00000000000A0000 -> 0x00000000000AFFFF fixed: 1 type: 6 // etc
ksm: CPU 1: ksm_init: MTRR Range: 0x00000000000F0000 -> 0x00000000000FFFFF fixed: 1 type: 6 // ...

wbenny commented 6 years ago

edited

asamy commented 6 years ago

Thanks!

Answer 1 · 2018-05-12T22:28:25.000Z

Can you send a patch?

Answer 2 · 2018-05-12T22:34:12.000Z

sorry, been quite lazy :) here you go:

From 176404f6f7ba873f0d314c65f4093f6a9fce04b7 Mon Sep 17 00:00:00 2001
From: Petr Benes <w.benny@outlook.com>
Date: Sun, 13 May 2018 00:39:27 +0200
Subject: [PATCH] fix wrongly computed mtrr range addresses

---
 mm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm.c b/mm.c
index d6ee8df..9e5ef0d 100644
--- a/mm.c
+++ b/mm.c
@@ -204,18 +204,18 @@ void mm_cache_mtrr_ranges(struct mtrr_range *ranges, int *range_count, u8 *def_t
 
 	if ((cap >> 8) & 1 && (def >> 10) & 1) {
 		/* Read fixed range MTRRs.  */
-		for (msr = __readmsr(MSR_MTRRfix64K_00000), offset = 0, base = 0;
-		     msr != 0; msr >>= 8, offset += 0x10000, base += offset)
+		for (msr = __readmsr(MSR_MTRRfix64K_00000), offset = 0x10000, base = 0;
+		     msr != 0; msr >>= 8, base += offset)
 			make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x10000 - 1);
 
-		for (val = MSR_MTRRfix16K_80000, offset = 0; val <= MSR_MTRRfix16K_A0000; ++val)
+		for (val = MSR_MTRRfix16K_80000, offset = 0x4000; val <= MSR_MTRRfix16K_A0000; ++val)
 			for (msr = __readmsr(val), base = 0x80000;
-			     msr != 0; msr >>= 8, offset += 0x4000, base += offset)
+			     msr != 0; msr >>= 8, base += offset)
 				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x4000 - 1);
 
-		for (val = MSR_MTRRfix4K_C0000, offset = 0; val <= MSR_MTRRfix4K_F8000; ++val)
+		for (val = MSR_MTRRfix4K_C0000, offset = 0x1000; val <= MSR_MTRRfix4K_F8000; ++val)
 			for (msr = __readmsr(val), base = 0xC0000;
-			     msr != 0; msr >>= 8, offset += 0x1000, base += offset)
+			     msr != 0; msr >>= 8, base += offset)
 				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x1000 - 1);
 	}
 
-- 
2.17.0.windows.1

Answer 3 · 2018-05-12T22:36:46.000Z

That's a diff not a patch :(
Generate with:
git format-patch HEAD~

Answer 4 · 2018-05-12T22:49:57.000Z

Applied, thanks!

Answer 5 · 2018-05-12T23:12:43.000Z

You probably also want to apply this. in_bounds checks for gpa < end, not gpa <= end. mm_cache_ram_ranges does this correctly.

From dcf30c903ab99f258c02f6a70f5096f5f5ae0cce Mon Sep 17 00:00:00 2001
From: Petr Benes <w.benny@outlook.com>
Date: Sun, 13 May 2018 01:11:01 +0200
Subject: [PATCH] fix off-by-one error in making of mtrr range

---
 mm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm.c b/mm.c
index 9e5ef0d..34a75ce 100644
--- a/mm.c
+++ b/mm.c
@@ -206,17 +206,17 @@ void mm_cache_mtrr_ranges(struct mtrr_range *ranges, int *range_count, u8 *def_t
 		/* Read fixed range MTRRs.  */
 		for (msr = __readmsr(MSR_MTRRfix64K_00000), offset = 0x10000, base = 0;
 		     msr != 0; msr >>= 8, base += offset)
-			make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x10000 - 1);
+			make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x10000);
 
 		for (val = MSR_MTRRfix16K_80000, offset = 0x4000; val <= MSR_MTRRfix16K_A0000; ++val)
 			for (msr = __readmsr(val), base = 0x80000;
 			     msr != 0; msr >>= 8, base += offset)
-				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x4000 - 1);
+				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x4000);
 
 		for (val = MSR_MTRRfix4K_C0000, offset = 0x1000; val <= MSR_MTRRfix4K_F8000; ++val)
 			for (msr = __readmsr(val), base = 0xC0000;
 			     msr != 0; msr >>= 8, base += offset)
-				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x1000 - 1);
+				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x1000);
 	}
 
 	for (i = 0; i < num_var; i++) {
-- 
2.17.0.windows.1

Answer 6 · 2018-05-12T23:14:59.000Z

Makes sense that way I guess. Thanks!

Answer 7 · 2018-05-12T23:20:06.000Z

Glad I've helped! Maybe that's the reason of the two bugs sitting in the issue page for so long.
Also, umm... I've edited the email in the last patch, I mistakenly used the one for the work. Ummm... do you think you can edit & force push that, please? :|

Answer 8 · 2018-05-12T23:24:04.000Z

Hah, I noticed that too, done.

Thanks!

Answer 9 · 2018-05-12T23:41:48.000Z

I swear this is the last one... :)

From fcc0e8cdb8701be7feb01542093d985ebc7a12cd Mon Sep 17 00:00:00 2001
From: Petr Benes <w.benny@outlook.com>
Date: Sun, 13 May 2018 01:40:43 +0200
Subject: [PATCH] another off-by-one error

---
 mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm.c b/mm.c
index 34a75ce..eef5511 100644
--- a/mm.c
+++ b/mm.c
@@ -229,7 +229,7 @@ void mm_cache_mtrr_ranges(struct mtrr_range *ranges, int *range_count, u8 *def_t
 		make_mtrr_range(&ranges[idx++], false,
 				base & 0xff,
 				base & PAGE_PA_MASK,
-				(base & PAGE_PA_MASK) + len - 1);
+				(base & PAGE_PA_MASK) + len);
 	}
 
 	*range_count = idx;
-- 
2.17.0.windows.1

Answer 10 · 2018-05-13T10:51:12.000Z

This fixes #23 and #22 right?

Answer 11 · 2018-05-13T11:40:34.000Z

Honestly, I don't know, I couldn't reproduce these bugs. Just a wild guess. But I was referring to them, yes. :) Maybe time to bump these issues up and summon the authors back to try it.

Answer 12 · 2018-07-23T23:18:05.000Z

Hi again, previous fixes were not sufficient. In the loop where you've been filling fixed MTRRs, the exit condition checked for msr != 0. But if fixed MSR contained eight EPT_MT_UNCACHABLE sub-ranges, the MSR value has been already 0 (therefore the whole loop has been skipped). I've checked that maybe ept_memory_type function accounts for it, but it doesn't (and on a second thought, it even can't).

So, here's another patch :)

From 20cfc80b251c6d1da5d516711fdbaae8e1a0e1d4 Mon Sep 17 00:00:00 2001
From: Petr Benes <w.benny@outlook.com>
Date: Tue, 24 Jul 2018 01:13:40 +0200
Subject: [PATCH] fix wrongly computed MTRR (again)

---
 mm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm.c b/mm.c
index eef5511..bf53c03 100644
--- a/mm.c
+++ b/mm.c
@@ -204,19 +204,19 @@ void mm_cache_mtrr_ranges(struct mtrr_range *ranges, int *range_count, u8 *def_t
 
 	if ((cap >> 8) & 1 && (def >> 10) & 1) {
 		/* Read fixed range MTRRs.  */
-		for (msr = __readmsr(MSR_MTRRfix64K_00000), offset = 0x10000, base = 0;
-		     msr != 0; msr >>= 8, base += offset)
-			make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x10000);
+		for (msr = __readmsr(MSR_MTRRfix64K_00000), offset = 0x10000, base = 0, i = 0;
+		     i < 8; msr >>= 8, base += offset, ++i)
+			make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + offset);
 
 		for (val = MSR_MTRRfix16K_80000, offset = 0x4000; val <= MSR_MTRRfix16K_A0000; ++val)
-			for (msr = __readmsr(val), base = 0x80000;
-			     msr != 0; msr >>= 8, base += offset)
-				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x4000);
+			for (msr = __readmsr(val), base = 0x80000, i = 0;
+			     i < 8; msr >>= 8, base += offset, ++i)
+				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + offset);
 
 		for (val = MSR_MTRRfix4K_C0000, offset = 0x1000; val <= MSR_MTRRfix4K_F8000; ++val)
-			for (msr = __readmsr(val), base = 0xC0000;
-			     msr != 0; msr >>= 8, base += offset)
-				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + 0x1000);
+			for (msr = __readmsr(val), base = 0xC0000, i = 0;
+			     i < 8; msr >>= 8, base += offset, ++i)
+				make_mtrr_range(&ranges[idx++], true, msr & 0xff, base, base + offset);
 	}
 
 	for (i = 0; i < num_var; i++) {
-- 
2.17.0.windows.1

Answer 13 · 2018-07-24T00:40:54.000Z

Well, it looks like variable MTRR computation is wrong as well:

From 10c7c3bba6be80ace724922f199d5ba206522abf Mon Sep 17 00:00:00 2001
From: Petr Benes <w.benny@outlook.com>
Date: Tue, 24 Jul 2018 02:39:58 +0200
Subject: [PATCH] fix wrongly computed MTRR (this time - variable)

---
 mm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm.c b/mm.c
index bf53c03..46be919 100644
--- a/mm.c
+++ b/mm.c
@@ -194,7 +194,7 @@ void mm_cache_mtrr_ranges(struct mtrr_range *ranges, int *range_count, u8 *def_t
 	int num_var;
 	int idx = 0;
 	int i;
-	u32 len;
+	u64 len;
 
 	def = __readmsr(MSR_MTRRdefType);
 	*def_type = def & 0xFF;
@@ -224,7 +224,7 @@ void mm_cache_mtrr_ranges(struct mtrr_range *ranges, int *range_count, u8 *def_t
 		if (!((msr >> 11) & 1))
 			continue;
 
-		len = 1 << __ffs64(msr & PAGE_PA_MASK);
+		len = 1ull << __ffs64(msr & PAGE_PA_MASK);
 		base = __readmsr(MSR_MTRR_PHYS_BASE + i * 2);
 		make_mtrr_range(&ranges[idx++], false,
 				base & 0xff,
-- 
2.17.0.windows.1

Answer 14 · 2018-07-24T17:34:42.000Z

Thanks, applied.