x86/mm/KASLR: Fix the wrong size of memory sections

kernel_randomize_memory() uses __PHYSICAL_MASK_SHIFT to calculate the maximum amount of system RAM supported. The size of the direct mapping section is obtained from the smaller one of the below two values:

(actual system RAM size + padding size) vs (max system RAM size supported)

This calculation is wrong since commit:
b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52").

In commit b83ce5ee91471d, __PHYSICAL_MASK_SHIFT was changed to be 52, regardless of whether it's using 4-level or 5-level page tables. It will always use 4 PB as the maximum amount of system RAM, even in 4-level paging mode where it should be 64 TB. Thus the size of the direct mapping section will always be the sum of the actual system RAM size plus the padding size.

Even when the amount of system RAM is 64 TB, the following layout will still be used. Obviously KALSR will be weakened significantly.

   |____|_______actual RAM_______|_padding_|______the rest_______|
   0            64TB                                            ~120TB

What we want is the following:

   |____|_______actual RAM_______|_________the rest______________|
   0            64TB                                            ~120TB

So the code should use MAX_PHYSMEM_BITS instead. Fix it by replacing __PHYSICAL_MASK_SHIFT with MAX_PHYSMEM_BITS.

Now, lemme make sure I understand exactly what you're fixing here:
you're fixing the case where CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING is not 0. Which is the case when CONFIG_MEMORY_HOTPLUG is enabled.

Yes, no?

If so, please extend the commit message with that fact because it is crucial and the last missing piece in the explanation.

Otherwise, when the padding is 0, the clamping:

        /* Adapt phyiscal memory region size based on available memory */
        if (memory_tb < kaslr_regions[0].size_tb)
                kaslr_regions[0].size_tb = memory_tb;

will "fix" the direct mapping section size.

I made a new one to add this fact, I can repost if it's OK to you.

Quoted 1 line · Baoquan He

I made a new one to add this fact, I can repost if it's OK to you.

No, it looks ok and I can take it from here.

Also, resending too often is annoying, as I'm sure you know. Try to stick to resending once a week.

kernel_randomize_memory() hardcodes the size of vmemmap section as 1 TB, to support the maximum amount of system RAM in 4-level paging mode, 64 TB.

However, 1 TB is not enough for vmemmap in 5-level paging mode. Assuming the size of struct page is 64 Bytes, to support 4 PB system RAM in 5-level, 64 TB of vmemmap area is needed. The wrong hardcoding may cause vmemmap stamping into the following cpu_entry_area section, if KASLR puts vmemmap very close to cpu_entry_area, and the actual area of vmemmap is much bigger than 1 TB.

So here calculate the actual size of vmemmap region, then align up to 1 TB boundary. In 4-level it's always 1 TB. In 5-level it's adjusted on demand. The current code reserves 0.5 PB for vmemmap in 5-level. In this new methor,

the left space can be saved to join randomization to increase the entropy.

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 +3 -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;
 
+	/**

Quoted 10 lines · Baoquan He

kernel_randomize_memory() hardcodes the size of vmemmap section as 1 TB, to support the maximum amount of system RAM in 4-level paging mode, 64 TB.

However, 1 TB is not enough for vmemmap in 5-level paging mode. Assuming the size of struct page is 64 Bytes, to support 4 PB system RAM in 5-level, 64 TB of vmemmap area is needed. The wrong hardcoding may cause vmemmap stamping into the following cpu_entry_area section, if KASLR puts vmemmap very close to cpu_entry_area, and the actual area of vmemmap is much bigger than 1 TB.

Quoted 3 lines

So here calculate the actual size of vmemmap region, then align up to 1 TB boundary. In 4-level it's always 1 TB. In 5-level it's adjusted on demand. The current code reserves 0.5 PB for vmemmap in 5-level. In this new methor,

								       ^^^^^^^

Please introduce a spellchecker into your patch creation workflow.

Quoted 35 lines

the left space can be saved to join randomization to increase the entropy.

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 +3 -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;
 
+	/**

arch/x86/mm/kaslr.c void __init kernel_randomize_memory(void) · line 123

Quoted 10 lines

arch/x86/mm/kaslr.c +7 -0

+	 * 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++)

Kirill, ack?

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Quoted 9 lines · Baoquan He

kernel_randomize_memory() hardcodes the size of vmemmap section as 1 TB, to support the maximum amount of system RAM in 4-level paging mode, 64 TB.

However, 1 TB is not enough for vmemmap in 5-level paging mode. Assuming the size of struct page is 64 Bytes, to support 4 PB system RAM in 5-level, 64 TB of vmemmap area is needed. The wrong hardcoding may cause vmemmap stamping into the following cpu_entry_area section, if KASLR puts vmemmap very close to cpu_entry_area, and the actual area of vmemmap is much bigger than 1 TB.

Kirill, ack?

I sent private mail to Kirill and Kees. Kirill haven't replied yet, he could be busy with something else as he doesn't show up recently on lkml.

Kees kindly replied, and said he couldn't find this mail thread. He told I can add his Reviewed-by, as he has acked this patchset in v2 thread. I just updated later to tune log and correct typos.
http://lkml.kernel.org/r/CAGXu5j+o4aSx9mMDJqTMOp-VrvWes-2YEwR1f29z8dm0rUfzGQ@mail.gmail.com

Quoted 3 lines · Baoquan He

I sent private mail to Kirill and Kees. Kirill haven't replied yet, he could be busy with something else as he doesn't show up recently on lkml.

I don't understand what the hurry is?

The merge window is imminent and we only pick obvious fixes. That doesn't qualify as such, AFAICT.

Quoted 5 lines

Kees kindly replied, and said he couldn't find this mail thread. He told I can add his Reviewed-by, as he has acked this patchset in v2 thread. I just updated later to tune log and correct typos.
http://lkml.kernel.org/r/CAGXu5j+o4aSx9mMDJqTMOp-VrvWes-2YEwR1f29z8dm0rUfzGQ@mail.gmail.com

Yes, when you get Reviewed-by:'s or other tags from reviewers, you *add* them to your next submission when the patch doesn't change in non-trivial fashion. You should know that...

kernel_randomize_memory() hardcodes the size of vmemmap section as 1 TB, to support the maximum amount of system RAM in 4-level paging mode, 64 TB.

kernel_randomize_memory() hardcodes the size of vmemmap section as 1 TB, to support the maximum amount of system RAM in 4-level paging mode, 64 TB.

However, 1 TB is not enough for vmemmap in 5-level paging mode. Assuming the size of struct page is 64 Bytes, to support 4 PB system RAM in 5-level, 64 TB of vmemmap area is needed. The wrong hardcoding may cause vmemmap stamping into the following cpu_entry_area section, if KASLR puts vmemmap very close to cpu_entry_area , and the actual area of vmemmap is much bigger than 1 TB.

So here calculate the actual size of vmemmap region, then align up to 1 TB boundary. In 4-level it's always 1 TB. In 5-level it's adjusted on demand. The current code reserves 0.5 PB for vmemmap in 5-level. In this new method, the left space can be saved to join randomization to increase the entropy.

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

---
v2->v3:
Fix typo Boris pointed out.

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

arch/x86/mm/kaslr.c +2 -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;

arch/x86/mm/kaslr.c +2 -0

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

arch/x86/mm/kaslr.c void __init kernel_randomize_memory(void) · line 113

Quoted 5 lines

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

@@ -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;
 
+	/**

Nit: that is weird style for inline comment.

arch/x86/mm/kaslr.c void __init kernel_randomize_memory(void) · line 118

Quoted 6 lines

arch/x86/mm/kaslr.c +5 -0

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

Hm. Don't we need to take into account alignment requirements for struct page here? I'm worried about some exotic debug kernel config where sizeof(struct page) doesn't satify __alignof__(struct page).

Quoted 6 lines

arch/x86/mm/kaslr.c +2 -0

+	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

 Kirill A. Shutemov

Quoted 9 lines

arch/x86/mm/kaslr.c void __init kernel_randomize_memory(void) · line 118

Quoted 5 lines

arch/x86/mm/kaslr.c +5 -0

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

Hm. Don't we need to take into account alignment requirements for struct page here? I'm worried about some exotic debug kernel config where sizeof(struct page) doesn't satify __alignof__(struct page).

I know sizeof(struct page) has handled its own struct alignment and padding.