Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrongly computed MTRR range addresses #25

Closed
1 of 3 tasks
wbenny opened this issue May 12, 2018 · 16 comments
Closed
1 of 3 tasks

Wrongly computed MTRR range addresses #25

wbenny opened this issue May 12, 2018 · 16 comments

Comments

@wbenny
Copy link
Contributor

wbenny commented May 12, 2018

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 // ...
@asamy
Copy link
Owner

asamy commented May 12, 2018

Can you send a patch?

@wbenny
Copy link
Contributor Author

wbenny commented May 12, 2018

sorry, been quite lazy :) here you go:

From 176404f6f7ba873f0d314c65f4093f6a9fce04b7 Mon Sep 17 00:00:00 2001
From: Petr Benes <[email protected]>
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

@asamy
Copy link
Owner

asamy commented May 12, 2018

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

@wbenny
Copy link
Contributor Author

wbenny commented May 12, 2018

edited

@asamy
Copy link
Owner

asamy commented May 12, 2018

Applied, thanks!

@wbenny
Copy link
Contributor Author

wbenny commented May 12, 2018

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

@asamy
Copy link
Owner

asamy commented May 12, 2018

Makes sense that way I guess. Thanks!

@wbenny
Copy link
Contributor Author

wbenny commented May 12, 2018

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? :|

@asamy
Copy link
Owner

asamy commented May 12, 2018

Hah, I noticed that too, done.

Thanks!

@wbenny
Copy link
Contributor Author

wbenny commented May 12, 2018

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

From fcc0e8cdb8701be7feb01542093d985ebc7a12cd Mon Sep 17 00:00:00 2001
From: Petr Benes <[email protected]>
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

@asamy
Copy link
Owner

asamy commented May 13, 2018

Thanks!

@asamy
Copy link
Owner

asamy commented May 13, 2018

This fixes #23 and #22 right?

@wbenny
Copy link
Contributor Author

wbenny commented May 13, 2018

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.

@wbenny
Copy link
Contributor Author

wbenny commented Jul 23, 2018

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

@wbenny
Copy link
Contributor Author

wbenny commented Jul 24, 2018

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

From 10c7c3bba6be80ace724922f199d5ba206522abf Mon Sep 17 00:00:00 2001
From: Petr Benes <[email protected]>
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

@asamy
Copy link
Owner

asamy commented Jul 24, 2018

Thanks, applied.

@asamy asamy closed this as completed Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants