x86/mm/KASLR: Fix two code bugs

PATCH Superseded Baoquan He 3 participants 9 messages Apr 4, 2019 – Apr 8, 2019

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Baoquan He

author

2019-04-04 02:03 UTC #0 lore raw

cover x86/mm/KASLR: Fix two code bugs

The fixes for these two bugs were carried in the earlier patchset, patch 4/6 and patch 5/6:

[PATCH v4 0/6] Several patches to fix code bugs, improve documents and clean up
http://lkml.kernel.org/r/[email protected]

Later, Thomas suggested posting bug fixing patches separately from those clean up patches. So send both of them in a separate patchset.

For another bug fix patch 6/6, it happened on SGI UV system. We need a specific SGI UV machine to reproduce and verify it, still discussing with Mike and Frank from HPE SGI about arranging a machine to test. That machine is very rare, still waiting. So defer that bug fix reposting later.

Baoquan He (2):
x86/mm/KASLR: Fix the wrong calculation of memory region initial size x86/mm/KASLR: Calculate the actual size of vmemmap region

 arch/x86/mm/kaslr.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Signature · 1 line

2.17.2

Baoquan He

author

2019-04-04 02:03 UTC #1 lore raw

PATCH 1/2 x86/mm/KASLR: Fix the wrong calculation of memory region initial size

In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate the initial size of the direct mapping region. This is correct in the old code where __PHYSICAL_MASK_SHIFT was equal to MAX_PHYSMEM_BITS, 46 bits, and only 4-level mode was supported.

Later, in commit:
b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52"), __PHYSICAL_MASK_SHIFT was changed to be always 52 bits, no matter it's 5-level or 4-level.

This is wrong for 4-level paging since it may cause randomness of KASLR being greatly weakened in 4-level. For KASLR, we compare the sum of RAM size and CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING with the size of the max RAM which can be supported by system, then choose the bigger one as the value to reserve space for the direct mapping region. The max RAM supported in 4-level is 64 TB according to MAX_PHYSMEM_BITS. However, here it's 4 PB in code to be compared with when __PHYSICAL_MASK_SHIFT is mistakenly used. E.g in a system owning 64 TB RAM, it will reserve 74 TB (which is 64 TB plus CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING). In fact it should reserve 64 TB according to the algorithm which is supposed to do. Obviously the extra 10 TB space should be saved to join randomization.

So, here MAX_PHYSMEM_BITS should be used instead. Fix it by replacing __PHYSICAL_MASK_SHIFT with MAX_PHYSMEM_BITS.

Acked-by Kirill A. Shutemov <[email protected]>

Reviewed-by Thomas Garnier <[email protected]>

Signed-off-by Baoquan He <[email protected]>

---
 arch/x86/mm/kaslr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/mm/kaslr.c +1 -1

index 9a8756517504..387d4ed25d7c 100644
@@ -94,7 +94,7 @@ void __init kernel_randomize_memory(void)
 	if (!kaslr_memory_enabled())
 		return;
 
-	kaslr_regions[0].size_tb = 1 << (__PHYSICAL_MASK_SHIFT - TB_SHIFT);
+	kaslr_regions[0].size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
 	kaslr_regions[1].size_tb = VMALLOC_SIZE_TB;
 
 	/*

Signature · 1 line

2.17.2

Borislav Petkov

maintainer

2019-04-05 16:59 UTC #2 lore raw

Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

Quoted 1 line · Baoquan He

In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate

What is "memory region KASLR"?

Quoted 9 lines

the initial size of the direct mapping region. This is correct in the old code where __PHYSICAL_MASK_SHIFT was equal to MAX_PHYSMEM_BITS, 46 bits, and only 4-level mode was supported.
Later, in commit:
b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52"), __PHYSICAL_MASK_SHIFT was changed to be always 52 bits, no matter it's 5-level or 4-level.

Quoted 11 lines

This is wrong for 4-level paging since it may cause randomness of KASLR being greatly weakened in 4-level. For KASLR, we compare the sum of RAM size and CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING with the size of the max RAM which can be supported by system, then choose the bigger one as the value to reserve space for the direct mapping region. The max RAM supported in 4-level is 64 TB according to MAX_PHYSMEM_BITS. However, here it's 4 PB in code to be compared with when __PHYSICAL_MASK_SHIFT is mistakenly used. E.g in a system owning 64 TB RAM, it will reserve 74 TB (which is 64 TB plus CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING). In fact it should reserve 64 TB according to the algorithm which is supposed to do. Obviously the extra 10 TB space should be saved to join randomization.

It is not a trivial situation you're trying to explain and that paragraph is very very hard to understand. I can only rhyme up what you're trying to say.

So please rewrite it using simple declarative sentences. Don't try to say three things in one sentence but say one thing in three sentences. Keep it simple.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Thomas Gleixner

2019-04-05 17:22 UTC #3 lore raw

Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

Quoted 26 lines · Borislav Petkov

Quoted 1 line · Baoquan He
In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate
What is "memory region KASLR"?
Quoted 9 lines
the initial size of the direct mapping region. This is correct in the old code where __PHYSICAL_MASK_SHIFT was equal to MAX_PHYSMEM_BITS, 46 bits, and only 4-level mode was supported.
Later, in commit:
b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52"), __PHYSICAL_MASK_SHIFT was changed to be always 52 bits, no matter it's 5-level or 4-level.
Quoted 11 lines
This is wrong for 4-level paging since it may cause randomness of KASLR being greatly weakened in 4-level. For KASLR, we compare the sum of RAM size and CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING with the size of the max RAM which can be supported by system, then choose the bigger one as the value to reserve space for the direct mapping region. The max RAM supported in 4-level is 64 TB according to MAX_PHYSMEM_BITS. However, here it's 4 PB in code to be compared with when __PHYSICAL_MASK_SHIFT is mistakenly used. E.g in a system owning 64 TB RAM, it will reserve 74 TB (which is 64 TB plus CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING). In fact it should reserve 64 TB according to the algorithm which is supposed to do. Obviously the extra 10 TB space should be saved to join randomization.

Quoted 7 lines

It is not a trivial situation you're trying to explain and that paragraph is very very hard to understand. I can only rhyme up what you're trying to say.
So please rewrite it using simple declarative sentences. Don't try to say three things in one sentence but say one thing in three sentences. Keep it simple.

For complex scenarios a simple ascii scheme is often helpful

Situation A

    ------- LIMIT1

    ------- LIMIT2
			<- unused area
    -------

    ------- 0

Situation B

    ------- LIMIT1


		
    ------- LIMIT2

    ------- 0

I was not trying to depict your problem, it's just a random thing, but you get the idea.

Thanks,

tglx

Baoquan He

author

2019-04-06 01:55 UTC #4 lore raw

Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

Quoted 58 lines · Thomas Gleixner

Quoted 26 lines · Borislav Petkov
Quoted 1 line · Baoquan He
In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate
What is "memory region KASLR"?
Quoted 9 lines
the initial size of the direct mapping region. This is correct in the old code where __PHYSICAL_MASK_SHIFT was equal to MAX_PHYSMEM_BITS, 46 bits, and only 4-level mode was supported.
Later, in commit:
b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52"), __PHYSICAL_MASK_SHIFT was changed to be always 52 bits, no matter it's 5-level or 4-level.
Quoted 11 lines
This is wrong for 4-level paging since it may cause randomness of KASLR being greatly weakened in 4-level. For KASLR, we compare the sum of RAM size and CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING with the size of the max RAM which can be supported by system, then choose the bigger one as the value to reserve space for the direct mapping region. The max RAM supported in 4-level is 64 TB according to MAX_PHYSMEM_BITS. However, here it's 4 PB in code to be compared with when __PHYSICAL_MASK_SHIFT is mistakenly used. E.g in a system owning 64 TB RAM, it will reserve 74 TB (which is 64 TB plus CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING). In fact it should reserve 64 TB according to the algorithm which is supposed to do. Obviously the extra 10 TB space should be saved to join randomization.
Quoted 7 lines
It is not a trivial situation you're trying to explain and that paragraph is very very hard to understand. I can only rhyme up what you're trying to say.
So please rewrite it using simple declarative sentences. Don't try to say three things in one sentence but say one thing in three sentences. Keep it simple.
For complex scenarios a simple ascii scheme is often helpful
Situation A
    ------- LIMIT1

    ------- LIMIT2
			<- unused area
    -------

    ------- 0
Situation B
    ------- LIMIT1


		
    ------- LIMIT2

    ------- 0

Quoted 2 lines

I was not trying to depict your problem, it's just a random thing, but you get the idea.

OK, got it. Will rewrite with simpler sentences, and some more understandable ways to depict. Thanks a lot.

Thanks
Baoquan

Baoquan He

author

2019-04-06 01:51 UTC #5 lore raw

Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

In reply to Borislav Petkov

Quoted 4 lines · Borislav Petkov

Quoted 1 line · Baoquan He
In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate
What is "memory region KASLR"?

It's KASLR happened in kernel_randomize_memory() of arch/x86/mm/kaslr.c . In fact, I don't know how to call it. Previously, I wrote it as mm KASLR, to distinguish from KASLR during kernel decompression. Ingo blamed the name, so I changed it to memory region KASLR. Seems Thomas Garnier called it KASLR for kernel memory regions in his original KASLR adding patch. May I call it 'KASLR for kernel memory regions', or 'KASLR for memory regions'?

Quoted 22 lines

Quoted 10 lines
the initial size of the direct mapping region. This is correct in the old code where __PHYSICAL_MASK_SHIFT was equal to MAX_PHYSMEM_BITS, 46 bits, and only 4-level mode was supported.
Later, in commit:
b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52"), __PHYSICAL_MASK_SHIFT was changed to be always 52 bits, no matter it's 5-level or 4-level.
Quoted 11 lines
This is wrong for 4-level paging since it may cause randomness of KASLR being greatly weakened in 4-level. For KASLR, we compare the sum of RAM size and CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING with the size of the max RAM which can be supported by system, then choose the bigger one as the value to reserve space for the direct mapping region. The max RAM supported in 4-level is 64 TB according to MAX_PHYSMEM_BITS. However, here it's 4 PB in code to be compared with when __PHYSICAL_MASK_SHIFT is mistakenly used. E.g in a system owning 64 TB RAM, it will reserve 74 TB (which is 64 TB plus CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING). In fact it should reserve 64 TB according to the algorithm which is supposed to do. Obviously the extra 10 TB space should be saved to join randomization.

Quoted 7 lines

It is not a trivial situation you're trying to explain and that paragraph is very very hard to understand. I can only rhyme up what you're trying to say.
So please rewrite it using simple declarative sentences. Don't try to say three things in one sentence but say one thing in three sentences. Keep it simple.

OK, will rewrite the whole patch log.

Borislav Petkov

maintainer

2019-04-06 04:43 UTC #6 lore raw

Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

Quoted 1 line · Baoquan He

It's KASLR happened in kernel_randomize_memory() of arch/x86/mm/kaslr.c .

What is "KASLR happened in"? This doesn't make any sense. When you look at that function, there's a comment above it:

/* Initialize base and padding for each memory region randomized with KASLR */

Do you mean, that, per chance?

Quoted 3 lines

In fact, I don't know how to call it. Previously, I wrote it as mm KASLR, to distinguish from KASLR during kernel decompression. Ingo blamed the name,

Of course he did, because it didn't make any sense to him either.

Quoted 4 lines

so I changed it to memory region KASLR. Seems Thomas
Garnier called it KASLR for kernel memory regions in his original KASLR adding patch. May I call it 'KASLR for kernel memory regions', or 'KASLR for memory regions'?

So you're fixing kaslr_regions[0].size_tb. It's base gets initialized to page_offset_base.

Now, if you look at

Documentation/x86/x86_64/mm.txt

it says there:

ffff888000000000 | -119.5 TB | ffffc87fffffffff | 64 TB | direct mapping of all physical memory (page_offset_base)

so that is the direct mapping memory region of all physical memory.

Now, you're apparently fixing its size.

Am I making sense and are you catching my drift?

You need to explain what you change in your commit messages in *understandable* english so that reviewer/committer or even a person who's not deeply involved in KASLR inner workings, can at least get an idea about what the commit message is talking about.

If you come up with strange constructs like "memory region KASLR" or "KASLR happened in" or "mm KASLR" which only make sense in your head, you're not doing anyone any favour.

Commit messages need to be very understandable when someone is looking at them months or even years from now. And you need to restrain yourself when you write them. You will appreciate that the first time you have to do git archeology, dig out an ancient commit and wonder why we did it this way.

Because we didn't document as good in previous years and our commits from the past suck big time.

Thanks!

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Baoquan He

author

2019-04-08 07:54 UTC #7 lore raw

Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

Quoted 30 lines · Borislav Petkov

Quoted 1 line · Baoquan He
It's KASLR happened in kernel_randomize_memory() of arch/x86/mm/kaslr.c .
What is "KASLR happened in"? This doesn't make any sense. When you look at that function, there's a comment above it:
/* Initialize base and padding for each memory region randomized with KASLR */
Do you mean, that, per chance?
Quoted 3 lines
In fact, I don't know how to call it. Previously, I wrote it as mm KASLR, to distinguish from KASLR during kernel decompression. Ingo blamed the name,
Of course he did, because it didn't make any sense to him either.
Quoted 4 lines
so I changed it to memory region KASLR. Seems Thomas
Garnier called it KASLR for kernel memory regions in his original KASLR adding patch. May I call it 'KASLR for kernel memory regions', or 'KASLR for memory regions'?
So you're fixing kaslr_regions[0].size_tb. It's base gets initialized to page_offset_base.
Now, if you look at
Documentation/x86/x86_64/mm.txt
it says there:

Quoted 7 lines

ffff888000000000 | -119.5 TB | ffffc87fffffffff | 64 TB | direct mapping of all physical memory (page_offset_base)
so that is the direct mapping memory region of all physical memory.
Now, you're apparently fixing its size.
Am I making sense and are you catching my drift?

Yes, makes sense. I will make it more specific, and use kernel_randomize_memory() instead to indicate the place where code issue is found out. Thanks.

Quoted 26 lines

You need to explain what you change in your commit messages in *understandable* english so that reviewer/committer or even a person who's not deeply involved in KASLR inner workings, can at least get an idea about what the commit message is talking about.
If you come up with strange constructs like "memory region KASLR" or "KASLR happened in" or "mm KASLR" which only make sense in your head, you're not doing anyone any favour.
Commit messages need to be very understandable when someone is looking at them months or even years from now. And you need to restrain yourself when you write them. You will appreciate that the first time you have to do git archeology, dig out an ancient commit and wonder why we did it this way.
Because we didn't document as good in previous years and our commits from the past suck big time.
Thanks!
Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Baoquan He

author

2019-04-04 02:03 UTC #8 lore raw

PATCH 2/2 x86/mm/KASLR: Calculate the actual size of vmemmap region

In reply to Baoquan He

Vmemmap region has different maximum size depending on paging mode. However, its size is hardcoded as 1TB during memory KASLR handling, this is not right for 5-level paging mode since it will cause vital stumping if vmemmap region is randomized to be very close to the following cpu_entry_area region and the actual size of vmemmap is much bigger than 1 TB.

So here calculate how many TB needed by the actual size of vmemmap region and align up to 1TB boundary. In 4-level the size will be 1 TB always since the max is 1 TB. In 5-level it's variable so that space can be saved for randomization.

Signed-off-by Baoquan He <[email protected]>

---
 arch/x86/mm/kaslr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

arch/x86/mm/kaslr.c +10 -1

index 387d4ed25d7c..4679a0075048 100644
@@ -52,7 +52,7 @@ static __initdata struct kaslr_memory_region {
 } kaslr_regions[] = {
 	{ &page_offset_base, 0 },
 	{ &vmalloc_base, 0 },
-	{ &vmemmap_base, 1 },
+	{ &vmemmap_base, 0 },
 };
 
 /* Get size in bytes used by the memory region */

@@ -78,6 +78,7 @@ void __init kernel_randomize_memory(void)
 	unsigned long rand, memory_tb;
 	struct rnd_state rand_state;
 	unsigned long remain_entropy;
+	unsigned long vmemmap_size;
 
 	vaddr_start = pgtable_l5_enabled() ? __PAGE_OFFSET_BASE_L5 : __PAGE_OFFSET_BASE_L4;
 	vaddr = vaddr_start;

@@ -109,6 +110,14 @@ void __init kernel_randomize_memory(void)
 	if (memory_tb < kaslr_regions[0].size_tb)
 		kaslr_regions[0].size_tb = memory_tb;
 
+	/**
+	 * Calculate how many TB vmemmap region needs, and aligned to
+	 * 1TB boundary.
+	 */
+	vmemmap_size = (kaslr_regions[0].size_tb << (TB_SHIFT - PAGE_SHIFT)) *
+		sizeof(struct page);
+	kaslr_regions[2].size_tb = DIV_ROUND_UP(vmemmap_size, 1UL << TB_SHIFT);
+
 	/* Calculate entropy available between regions */
 	remain_entropy = vaddr_end - vaddr_start;
 	for (i = 0; i < ARRAY_SIZE(kaslr_regions); i++)

Signature · 1 line

2.17.2