[PATCH 0/2] x86/mm/KASLR: Fix two code bugs

[PATCH 0/2] x86/mm/KASLR: Fix two code bugs

[Baoquan He]

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/20190314094645.4883-1-bhe@redhat.com

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

-- 
2.17.2

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

[Baoquan He]

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 <kirill.shutemov@linux.intel.com>
Reviewed-by: Thomas Garnier <thgarnie@google.com>
Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/mm/kaslr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 9a8756517504..387d4ed25d7c 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -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;
 
 	/*
-- 
2.17.2

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

[Borislav Petkov]

On Thu, Apr 04, 2019 at 10:03:13AM +0800, Baoquan He wrote:
> In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate

What is "memory region KASLR"?

> 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.

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.

-- 
Regards/Gruss,
    Boris.

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

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

[Thomas Gleixner]

On Fri, 5 Apr 2019, Borislav Petkov wrote:
> On Thu, Apr 04, 2019 at 10:03:13AM +0800, Baoquan He wrote:
> > In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate
> 
> What is "memory region KASLR"?
> 
> > 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.
> 
> 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

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

[Baoquan He]

On 04/05/19 at 07:22pm, Thomas Gleixner wrote:
> On Fri, 5 Apr 2019, Borislav Petkov wrote:
> > On Thu, Apr 04, 2019 at 10:03:13AM +0800, Baoquan He wrote:
> > > In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate
> > 
> > What is "memory region KASLR"?
> > 
> > > 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.
> > 
> > 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.

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

Thanks
Baoquan

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

[Baoquan He]

On 04/05/19 at 06:58pm, Borislav Petkov wrote:
> On Thu, Apr 04, 2019 at 10:03:13AM +0800, Baoquan He wrote:
> > 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'?

> 
> > 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.
> 
> 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.

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

[Borislav Petkov]

On Sat, Apr 06, 2019 at 09:51:19AM +0800, Baoquan He wrote:
> 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?

> 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.

> 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!

-- 
Regards/Gruss,
    Boris.

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

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

[Baoquan He]

On 04/06/19 at 06:43am, Borislav Petkov wrote:
> On Sat, Apr 06, 2019 at 09:51:19AM +0800, Baoquan He wrote:
> > 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?
> 
> > 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.
> 
> > 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?

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.

> 
> 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!
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

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

[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 <bhe@redhat.com>
---
 arch/x86/mm/kaslr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 387d4ed25d7c..4679a0075048 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -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++)
-- 
2.17.2