[PATCH v4 0/3] x86/64/KASLR: Change kernel mapping size to 1G unconditionally

[PATCH v4 0/3] x86/64/KASLR: Change kernel mapping size to 1G unconditionally

[Baoquan He]

This is v4 post.

In the previous 3 versions I tried to detect and determine kernel image
mapping size at runtime for x86_64. With this the inconsistency between
KASLR code is not compiled in by disabling CONFIG_RANDOMIZE_BASE and
KASLR code is compiled in but add 'nokaslr' kernel option can be fixed.

When review v3 Boris suggested we should change kernel mapping size to
1G directly, but not an option. Kees explained he made kernel mapping
size as an option mainly because he woried about kernel module space.
He said it wasn't clear to him at the time if enough space remained for
modules in all use-cases. Then Boris pointed out that practically kaslr
will be enabled on the majority of the systems anyway, and the reduction
of kernel modules mapping space has been there for a while now, if so we
probably whould've heard complaints already. Kees agreed.

So in this version of post change kernel mapping size of x86 64 to 1G
as Boris suggested. Then the inconsistency will naturally disappear.

And I still keep patch 1/3 which introduces the new constant
KERNEL_MAPPING_SIZE. And let KERNEL_IMAGE_SIZE stay for restricting kernel
image size during linking stage.


v3->v4:
    Change kernel mapping size to 1G unconditionally as Boris suggested.

v2->v3:
    Boris pointed out patch log is not good for reviewing and understanding.
    So split the old patch 2/2 into 2 parts and rewrite the patch log,
    patch 2/3 is introducing the new constant KERNEL_MAPPING_SIZE which
    differs from the old KERNEL_IMAGE_SIZE, patch 3/3 gets the kernel mapping
    size at runtime.

v1->v2:
    Kbuild test threw build warning because of code bug.


Baoquan He (3):
  x86: Introduce a new constant KERNEL_MAPPING_SIZE
  x86/64/KASLR: Change kernel mapping size to 1G unconditionally
  x86/64/doc: Update the ranges of kernel text and modules mapping

 Documentation/x86/x86_64/mm.txt         |  4 ++--
 arch/x86/boot/compressed/kaslr.c        | 10 +++++-----
 arch/x86/include/asm/page_32_types.h    |  6 ++++++
 arch/x86/include/asm/page_64_types.h    | 17 ++++++++---------
 arch/x86/include/asm/pgtable_64_types.h |  2 +-
 arch/x86/kernel/head64.c                |  4 ++--
 arch/x86/kernel/head_64.S               | 12 +++++-------
 arch/x86/kernel/machine_kexec_64.c      |  2 +-
 arch/x86/mm/init_64.c                   |  2 +-
 arch/x86/mm/physaddr.c                  |  6 +++---
 10 files changed, 34 insertions(+), 31 deletions(-)

-- 
2.5.5

[PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

In x86, KERNEL_IMAGE_SIZE is used to limit the size of kernel image in
running space, but also represents the size of kernel image mapping area.
This looks good when kernel virtual address is invariable inside 512M
area and kernel image size is not bigger than 512M.

Along with the adding of kaslr, in x86_64 the area of kernel mapping is
extended up another 512M. It becomes improper to let KERNEL_IMAGE_SIZE
alone still play two roles now.

So introduce a new constant KERNEL_MAPPING_SIZE to represent the size of
kernel mapping area. Let KERNEL_IMAGE_SIZE be as its name is saying. In
x86_32 though kernel image size is the same as kernel mapping size, for
generic handling in kaslr.c KERNEL_MAPPING_SIZE is also introduced.

In this patch, just add KERNEL_MAPPING_SIZE and replace KERNEL_IMAGE_SIZE
with it in the relevant places. No functional change.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/kaslr.c        | 10 +++++-----
 arch/x86/include/asm/page_32_types.h    |  6 ++++++
 arch/x86/include/asm/page_64_types.h    | 12 +++++++++---
 arch/x86/include/asm/pgtable_64_types.h |  2 +-
 arch/x86/kernel/head64.c                |  4 ++--
 arch/x86/kernel/head_64.S               |  2 +-
 arch/x86/kernel/machine_kexec_64.c      |  2 +-
 arch/x86/mm/init_64.c                   |  2 +-
 arch/x86/mm/physaddr.c                  |  6 +++---
 9 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index a66854d..6d2424e 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -311,7 +311,7 @@ static void process_e820_entry(struct e820entry *entry,
 		return;
 
 	/* On 32-bit, ignore entries entirely above our maximum. */
-	if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
+	if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_MAPPING_SIZE)
 		return;
 
 	/* Ignore entries entirely below our minimum. */
@@ -341,8 +341,8 @@ static void process_e820_entry(struct e820entry *entry,
 
 		/* On 32-bit, reduce region size to fit within max size. */
 		if (IS_ENABLED(CONFIG_X86_32) &&
-		    region.start + region.size > KERNEL_IMAGE_SIZE)
-			region.size = KERNEL_IMAGE_SIZE - region.start;
+		    region.start + region.size > KERNEL_MAPPING_SIZE)
+			region.size = KERNEL_MAPPING_SIZE - region.start;
 
 		/* Return if region can't contain decompressed kernel */
 		if (region.size < image_size)
@@ -408,9 +408,9 @@ static unsigned long find_random_virt_addr(unsigned long minimum,
 	/*
 	 * There are how many CONFIG_PHYSICAL_ALIGN-sized slots
 	 * that can hold image_size within the range of minimum to
-	 * KERNEL_IMAGE_SIZE?
+	 * KERNEL_MAPPING_SIZE?
 	 */
-	slots = (KERNEL_IMAGE_SIZE - minimum - image_size) /
+	slots = (KERNEL_MAPPING_SIZE - minimum - image_size) /
 		 CONFIG_PHYSICAL_ALIGN + 1;
 
 	random_addr = kaslr_get_random_long("Virtual") % slots;
diff --git a/arch/x86/include/asm/page_32_types.h b/arch/x86/include/asm/page_32_types.h
index 3bae496..e93de86 100644
--- a/arch/x86/include/asm/page_32_types.h
+++ b/arch/x86/include/asm/page_32_types.h
@@ -42,6 +42,12 @@
  */
 #define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
 
+/*
+ * Kernel mapping size is limited to 512 MB which is equal to kernel image
+ * size.
+ */
+#define KERNEL_MAPPING_SIZE	KERNEL_IMAGE_SIZE
+
 #ifndef __ASSEMBLY__
 
 /*
diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 9215e05..24c9098 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -50,16 +50,22 @@
 #define __VIRTUAL_MASK_SHIFT	47
 
 /*
- * Kernel image size is limited to 1GiB due to the fixmap living in the
+ * Kernel image size is limited to 512 MB. The kernel code+data+bss
+ * must not be bigger than that.
+ */
+#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
+
+/*
+ * Kernel mapping size is limited to 1GiB due to the fixmap living in the
  * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Use
  * 512MiB by default, leaving 1.5GiB for modules once the page tables
  * are fully set up. If kernel ASLR is configured, it can extend the
  * kernel page table mapping, reducing the size of the modules area.
  */
 #if defined(CONFIG_RANDOMIZE_BASE)
-#define KERNEL_IMAGE_SIZE	(1024 * 1024 * 1024)
+#define KERNEL_MAPPING_SIZE	(1024 * 1024 * 1024)
 #else
-#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
+#define KERNEL_MAPPING_SIZE	(512 * 1024 * 1024)
 #endif
 
 #endif /* _ASM_X86_PAGE_64_DEFS_H */
diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
index 3a26420..a357050 100644
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -66,7 +66,7 @@ typedef struct { pteval_t pte; } pte_t;
 #define VMEMMAP_START	__VMEMMAP_BASE
 #endif /* CONFIG_RANDOMIZE_MEMORY */
 #define VMALLOC_END	(VMALLOC_START + _AC((VMALLOC_SIZE_TB << 40) - 1, UL))
-#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
+#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_MAPPING_SIZE)
 #define MODULES_END      _AC(0xffffffffff000000, UL)
 #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
 #define ESPFIX_PGD_ENTRY _AC(-2, UL)
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 54a2372..7484d86 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -139,8 +139,8 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
 	 * area mappings. (these are purely build-time and produce no code)
 	 */
 	BUILD_BUG_ON(MODULES_VADDR < __START_KERNEL_map);
-	BUILD_BUG_ON(MODULES_VADDR - __START_KERNEL_map < KERNEL_IMAGE_SIZE);
-	BUILD_BUG_ON(MODULES_LEN + KERNEL_IMAGE_SIZE > 2*PUD_SIZE);
+	BUILD_BUG_ON(MODULES_VADDR - __START_KERNEL_map < KERNEL_MAPPING_SIZE);
+	BUILD_BUG_ON(MODULES_LEN + KERNEL_MAPPING_SIZE > 2*PUD_SIZE);
 	BUILD_BUG_ON((__START_KERNEL_map & ~PMD_MASK) != 0);
 	BUILD_BUG_ON((MODULES_VADDR & ~PMD_MASK) != 0);
 	BUILD_BUG_ON(!(MODULES_VADDR > __START_KERNEL));
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index b467b14..cdfe4dc 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -468,7 +468,7 @@ NEXT_PAGE(level2_kernel_pgt)
 	 *  too.)
 	 */
 	PMDS(0, __PAGE_KERNEL_LARGE_EXEC,
-		KERNEL_IMAGE_SIZE/PMD_SIZE)
+		KERNEL_MAPPING_SIZE/PMD_SIZE)
 
 NEXT_PAGE(level2_fixmap_pgt)
 	.fill	506,8,0
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 307b1f4..817e342 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -337,7 +337,7 @@ void arch_crash_save_vmcoreinfo(void)
 #endif
 	vmcoreinfo_append_str("KERNELOFFSET=%lx\n",
 			      kaslr_offset());
-	VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
+	VMCOREINFO_NUMBER(KERNEL_MAPPING_SIZE);
 }
 
 /* arch-dependent functionality related to kexec file-based syscall */
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index af85b68..57fdea5 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -297,7 +297,7 @@ void __init init_extra_mapping_uc(unsigned long phys, unsigned long size)
 void __init cleanup_highmap(void)
 {
 	unsigned long vaddr = __START_KERNEL_map;
-	unsigned long vaddr_end = __START_KERNEL_map + KERNEL_IMAGE_SIZE;
+	unsigned long vaddr_end = __START_KERNEL_map + KERNEL_MAPPING_SIZE;
 	unsigned long end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1;
 	pmd_t *pmd = level2_kernel_pgt;
 
diff --git a/arch/x86/mm/physaddr.c b/arch/x86/mm/physaddr.c
index cfc3b91..c0b70fc 100644
--- a/arch/x86/mm/physaddr.c
+++ b/arch/x86/mm/physaddr.c
@@ -18,7 +18,7 @@ unsigned long __phys_addr(unsigned long x)
 	if (unlikely(x > y)) {
 		x = y + phys_base;
 
-		VIRTUAL_BUG_ON(y >= KERNEL_IMAGE_SIZE);
+		VIRTUAL_BUG_ON(y >= KERNEL_MAPPING_SIZE);
 	} else {
 		x = y + (__START_KERNEL_map - PAGE_OFFSET);
 
@@ -35,7 +35,7 @@ unsigned long __phys_addr_symbol(unsigned long x)
 	unsigned long y = x - __START_KERNEL_map;
 
 	/* only check upper bounds since lower bounds will trigger carry */
-	VIRTUAL_BUG_ON(y >= KERNEL_IMAGE_SIZE);
+	VIRTUAL_BUG_ON(y >= KERNEL_MAPPING_SIZE);
 
 	return y + phys_base;
 }
@@ -50,7 +50,7 @@ bool __virt_addr_valid(unsigned long x)
 	if (unlikely(x > y)) {
 		x = y + phys_base;
 
-		if (y >= KERNEL_IMAGE_SIZE)
+		if (y >= KERNEL_MAPPING_SIZE)
 			return false;
 	} else {
 		x = y + (__START_KERNEL_map - PAGE_OFFSET);
-- 
2.5.5

[PATCH v4 2/3] x86/64/KASLR: Change kernel mapping size to 1G unconditionally

[Baoquan He]

The current KASLR changes kernel mapping size from 512M to 1G as long
as CONFIG_RANDOMIZE_BASE is enabled, though "nokaslr" kernel option is
added. This is buggy. When people specify "nokaslr", whether KASLR code
compiled in or not, they expect to see no KASLR change at all, including
the size of kernel mapping area and module mapping area.

Kees explained the only reason he made kernel mapping size as an option
was for kernel module space. It wasn't clear at the time if enough space
remained for modules in all use-cases.

Boris suggested we can make the kernel mapping 1G unconditionally since
practically kaslr will be enabled on the majority of the systems anyway,
so we will have 1G text mapping size on most. And he further pointed out
that: [Quote his words as follows]
"""""
Realistically, on a typical bigger machine, the modules take up
something like <10M:

$ lsmod | awk '{ sum +=$2 } END { print sum }'
7188480

so I'm not really worried if we reduce it by default to 1G. Besides, the
reduction has been there for a while now - since CONFIG_RANDOMIZE_BASE -
so we probably would've heard complaints already...
"""""

Hence in this patch change kernel mapping size to 1G unconditionally.

Signed-off-by: Baoquan He <bhe@redhat.com>
Suggested-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/page_64_types.h |  9 +--------
 arch/x86/kernel/head_64.S            | 10 ++++------
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 24c9098..4120cfe 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -57,15 +57,8 @@
 
 /*
  * Kernel mapping size is limited to 1GiB due to the fixmap living in the
- * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Use
- * 512MiB by default, leaving 1.5GiB for modules once the page tables
- * are fully set up. If kernel ASLR is configured, it can extend the
- * kernel page table mapping, reducing the size of the modules area.
+ * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S).
  */
-#if defined(CONFIG_RANDOMIZE_BASE)
 #define KERNEL_MAPPING_SIZE	(1024 * 1024 * 1024)
-#else
-#define KERNEL_MAPPING_SIZE	(512 * 1024 * 1024)
-#endif
 
 #endif /* _ASM_X86_PAGE_64_DEFS_H */
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index cdfe4dc..3cc6dc6 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -458,14 +458,12 @@ NEXT_PAGE(level3_kernel_pgt)
 
 NEXT_PAGE(level2_kernel_pgt)
 	/*
-	 * 512 MB kernel mapping. We spend a full page on this pagetable
-	 * anyway.
+	 * 1 GB kernel mapping. We spend a full page on this pagetable.
 	 *
-	 * The kernel code+data+bss must not be bigger than that.
+	 * The kernel image size including code+data+bss must not be bigger
+	 * than this.
 	 *
-	 * (NOTE: at +512MB starts the module area, see MODULES_VADDR.
-	 *  If you want to increase this then increase MODULES_VADDR
-	 *  too.)
+	 * (NOTE: at +1GB starts the module area, see MODULES_VADDR.
 	 */
 	PMDS(0, __PAGE_KERNEL_LARGE_EXEC,
 		KERNEL_MAPPING_SIZE/PMD_SIZE)
-- 
2.5.5

[PATCH v4 3/3] x86/64/doc: Update the ranges of kernel text and modules mapping

[Baoquan He]

The size of kernel mapping range has been increased to 1G unconditionally.
Accordingly kernel modules mapping is shrunk to 1G. So update the
description in document.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 Documentation/x86/x86_64/mm.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/x86/x86_64/mm.txt b/Documentation/x86/x86_64/mm.txt
index 5724092..b7597a1 100644
--- a/Documentation/x86/x86_64/mm.txt
+++ b/Documentation/x86/x86_64/mm.txt
@@ -18,8 +18,8 @@ ffffff0000000000 - ffffff7fffffffff (=39 bits) %esp fixup stacks
 ... unused hole ...
 ffffffef00000000 - fffffffeffffffff (=64 GB) EFI region mapping space
 ... unused hole ...
-ffffffff80000000 - ffffffff9fffffff (=512 MB)  kernel text mapping, from phys 0
-ffffffffa0000000 - ffffffffff5fffff (=1526 MB) module mapping space
+ffffffff80000000 - ffffffffbfffffff (=1024 MB)  kernel text mapping, from phys 0
+ffffffffc0000000 - ffffffffff5fffff (=1014 MB) module mapping space
 ffffffffff600000 - ffffffffffdfffff (=8 MB) vsyscalls
 ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole
 
-- 
2.5.5

Re: [PATCH v4 0/3] x86/64/KASLR: Change kernel mapping size to 1G unconditionally

[Kees Cook]

On Thu, Feb 2, 2017 at 4:54 AM, Baoquan He <bhe@redhat.com> wrote:
> This is v4 post.
>
> In the previous 3 versions I tried to detect and determine kernel image
> mapping size at runtime for x86_64. With this the inconsistency between
> KASLR code is not compiled in by disabling CONFIG_RANDOMIZE_BASE and
> KASLR code is compiled in but add 'nokaslr' kernel option can be fixed.
>
> When review v3 Boris suggested we should change kernel mapping size to
> 1G directly, but not an option. Kees explained he made kernel mapping
> size as an option mainly because he woried about kernel module space.
> He said it wasn't clear to him at the time if enough space remained for
> modules in all use-cases. Then Boris pointed out that practically kaslr
> will be enabled on the majority of the systems anyway, and the reduction
> of kernel modules mapping space has been there for a while now, if so we
> probably whould've heard complaints already. Kees agreed.
>
> So in this version of post change kernel mapping size of x86 64 to 1G
> as Boris suggested. Then the inconsistency will naturally disappear.
>
> And I still keep patch 1/3 which introduces the new constant
> KERNEL_MAPPING_SIZE. And let KERNEL_IMAGE_SIZE stay for restricting kernel
> image size during linking stage.
>
>
> v3->v4:
>     Change kernel mapping size to 1G unconditionally as Boris suggested.

This looks good to me.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>
> v2->v3:
>     Boris pointed out patch log is not good for reviewing and understanding.
>     So split the old patch 2/2 into 2 parts and rewrite the patch log,
>     patch 2/3 is introducing the new constant KERNEL_MAPPING_SIZE which
>     differs from the old KERNEL_IMAGE_SIZE, patch 3/3 gets the kernel mapping
>     size at runtime.
>
> v1->v2:
>     Kbuild test threw build warning because of code bug.
>
>
> Baoquan He (3):
>   x86: Introduce a new constant KERNEL_MAPPING_SIZE
>   x86/64/KASLR: Change kernel mapping size to 1G unconditionally
>   x86/64/doc: Update the ranges of kernel text and modules mapping
>
>  Documentation/x86/x86_64/mm.txt         |  4 ++--
>  arch/x86/boot/compressed/kaslr.c        | 10 +++++-----
>  arch/x86/include/asm/page_32_types.h    |  6 ++++++
>  arch/x86/include/asm/page_64_types.h    | 17 ++++++++---------
>  arch/x86/include/asm/pgtable_64_types.h |  2 +-
>  arch/x86/kernel/head64.c                |  4 ++--
>  arch/x86/kernel/head_64.S               | 12 +++++-------
>  arch/x86/kernel/machine_kexec_64.c      |  2 +-
>  arch/x86/mm/init_64.c                   |  2 +-
>  arch/x86/mm/physaddr.c                  |  6 +++---
>  10 files changed, 34 insertions(+), 31 deletions(-)
>
> --
> 2.5.5
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Borislav Petkov]

On Thu, Feb 02, 2017 at 08:54:35PM +0800, Baoquan He wrote:
> In x86, KERNEL_IMAGE_SIZE is used to limit the size of kernel image in
> running space, but also represents the size of kernel image mapping area.
> This looks good when kernel virtual address is invariable inside 512M
> area and kernel image size is not bigger than 512M.
> 
> Along with the adding of kaslr, in x86_64 the area of kernel mapping is
> extended up another 512M. It becomes improper to let KERNEL_IMAGE_SIZE
> alone still play two roles now.
> 
> So introduce a new constant KERNEL_MAPPING_SIZE to represent the size of
> kernel mapping area. Let KERNEL_IMAGE_SIZE be as its name is saying. In
> x86_32 though kernel image size is the same as kernel mapping size, for
> generic handling in kaslr.c KERNEL_MAPPING_SIZE is also introduced.
> 
> In this patch, just add KERNEL_MAPPING_SIZE and replace KERNEL_IMAGE_SIZE
> with it in the relevant places. No functional change.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/kaslr.c        | 10 +++++-----
>  arch/x86/include/asm/page_32_types.h    |  6 ++++++
>  arch/x86/include/asm/page_64_types.h    | 12 +++++++++---
>  arch/x86/include/asm/pgtable_64_types.h |  2 +-
>  arch/x86/kernel/head64.c                |  4 ++--
>  arch/x86/kernel/head_64.S               |  2 +-
>  arch/x86/kernel/machine_kexec_64.c      |  2 +-
>  arch/x86/mm/init_64.c                   |  2 +-
>  arch/x86/mm/physaddr.c                  |  6 +++---
>  9 files changed, 29 insertions(+), 17 deletions(-)

...

> diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
> index 9215e05..24c9098 100644
> --- a/arch/x86/include/asm/page_64_types.h
> +++ b/arch/x86/include/asm/page_64_types.h
> @@ -50,16 +50,22 @@
>  #define __VIRTUAL_MASK_SHIFT	47
>  
>  /*
> - * Kernel image size is limited to 1GiB due to the fixmap living in the
> + * Kernel image size is limited to 512 MB. The kernel code+data+bss

This is not what it said there before. With your change you have:

- 0
.
.
.
- 512 - KERNEL_IMAGE_SIZE
.
.
.
- 1024 - KERNEL_MAPPING_SIZE

and KERNEL_IMAGE_SIZE is not limited to 512Mb but it is "Use 512Mib by
default". And we do enforce that in various places like in the linker
script assertions but there's some headroom open in the upper 512Mib if
needed.

KERNEL_MAPPING_SIZE OTOH is the one limited to 1G due to the fixmap L2
PGT...

> + * must not be bigger than that.
> + */
> +#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
> +
> +/*
> + * Kernel mapping size is limited to 1GiB due to the fixmap living in the
>   * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Use
>   * 512MiB by default, leaving 1.5GiB for modules once the page tables
>   * are fully set up. If kernel ASLR is configured, it can extend the
>   * kernel page table mapping, reducing the size of the modules area.
>   */
>  #if defined(CONFIG_RANDOMIZE_BASE)
> -#define KERNEL_IMAGE_SIZE	(1024 * 1024 * 1024)
> +#define KERNEL_MAPPING_SIZE	(1024 * 1024 * 1024)
>  #else
> -#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
> +#define KERNEL_MAPPING_SIZE	(512 * 1024 * 1024)
>  #endif

... and since you're adding that define now, fixup the comments in this
patch too, to explain what they mean.

Also, I'd like for the text to say that both defines are dependent in
the sense that IMAGE_SIZE <= MAPPING_SIZE so that people know what's
going on and which is which.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

Hi Boris,

Thanks a lot for your comments, sorry so late to reply!

On 02/14/17 at 06:32pm, Borislav Petkov wrote:
> > diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
> > index 9215e05..24c9098 100644
> > --- a/arch/x86/include/asm/page_64_types.h
> > +++ b/arch/x86/include/asm/page_64_types.h
> > @@ -50,16 +50,22 @@
> >  #define __VIRTUAL_MASK_SHIFT	47
> >  
> >  /*
> > - * Kernel image size is limited to 1GiB due to the fixmap living in the
> > + * Kernel image size is limited to 512 MB. The kernel code+data+bss
> 
> This is not what it said there before. With your change you have:
> 
> - 0
> .
> .
> .
> - 512 - KERNEL_IMAGE_SIZE
> .
> .
> .
> - 1024 - KERNEL_MAPPING_SIZE
> 
> and KERNEL_IMAGE_SIZE is not limited to 512Mb but it is "Use 512Mib by
> default". And we do enforce that in various places like in the linker
> script assertions but there's some headroom open in the upper 512Mib if
> needed.

Before below commit merged, it said:

 * Kernel image size is limited to 512 MB (see level2_kernel_pgt in
 * arch/x86/kernel/head_64.S), and it is mapped here:
 */
#define KERNEL_IMAGE_SIZE      (512 * 1024 * 1024)

commit 6145cfe3 ("x86, kaslr: Raise the maximum virtual address to -1
GiB on x86_64")

So you can see KERNEL_IMAGE_SIZE is a constant value and not optional.
Then Kees posted above commit, The default mentioned in "Use 512Mib by
default" should be KERNEL_IMAGE_SIZE in the case that kaslr code not
compiled in, 512M, which is relative to the case that kaslr code
compiled in, 1G. In fact, it's meaning the kernel mapping size defaults
to 512M, and will change to 1G if CONFIG_RANDOMIZE_BASE is chosen.

Seems in kernel only linker script checks kernel image size as below:

. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),                                                                                                  
           "kernel image bigger than KERNEL_IMAGE_SIZE");

In arch/x86/kernel/head64.c, it's more likely checking the kernel
mapping size, just because they share the same constant,
KERNEL_IMAGE_SIZE.

About headroom, in boot/compressed/kaslr.c kernel iamge size is
considered, including head room. Here I am not quite sure. Do you mean:
KERNEL_IMAGE_SIZE use 512Mib by default, only kerel image size is
limited to 512M which is (_end - _text) since linker script will check
it. While with headroom, it could be bigger than 512M if needed.

Am I right on understanding it?

> 
> KERNEL_MAPPING_SIZE OTOH is the one limited to 1G due to the fixmap L2
> PGT...
> 
> > + * must not be bigger than that.
> > + */
> > +#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
> > +
> > +/*
> > + * Kernel mapping size is limited to 1GiB due to the fixmap living in the
> >   * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Use
> >   * 512MiB by default, leaving 1.5GiB for modules once the page tables
> >   * are fully set up. If kernel ASLR is configured, it can extend the
> >   * kernel page table mapping, reducing the size of the modules area.
> >   */
> >  #if defined(CONFIG_RANDOMIZE_BASE)
> > -#define KERNEL_IMAGE_SIZE	(1024 * 1024 * 1024)
> > +#define KERNEL_MAPPING_SIZE	(1024 * 1024 * 1024)
> >  #else
> > -#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
> > +#define KERNEL_MAPPING_SIZE	(512 * 1024 * 1024)
> >  #endif
> 
> ... and since you're adding that define now, fixup the comments in this
> patch too, to explain what they mean.

Yes, agree, this makes it clearer. Will do.

> 
> Also, I'd like for the text to say that both defines are dependent in
> the sense that IMAGE_SIZE <= MAPPING_SIZE so that people know what's
> going on and which is which.

It makes sense, will do. 

Thanks for great suggestion!

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Borislav Petkov]

On Sun, Feb 26, 2017 at 12:09:08PM +0800, Baoquan He wrote:
> Am I right on understanding it?

That's exactly what I mean: KERNEL_IMAGE_SIZE is 512M by default but
we're not hard-constrained to it - we're hard-constrained to a 1G limit
as this is the 1G which is covered by level2_kernel_pgt.

And in thinking about this more, I know I suggested making the
KERNEL_IMAGE_SIZE by default 1G in order to simplify things.

But you're adding another KERNEL_MAPPING_SIZE which confuses things
more. And I fail to see why we absolutely need it.

So we suggest kernel image size should be 512M but then we still will
be using a whole 1G mapping for it anyway and a whole page of PMDs at
level2_kernel_pgt.

So why even bother?

Just make it 1G and don't introduce anything new.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 12:43pm, Borislav Petkov wrote:
> On Sun, Feb 26, 2017 at 12:09:08PM +0800, Baoquan He wrote:
> > Am I right on understanding it?
> 
> That's exactly what I mean: KERNEL_IMAGE_SIZE is 512M by default but
> we're not hard-constrained to it - we're hard-constrained to a 1G limit
> as this is the 1G which is covered by level2_kernel_pgt.
> 
> And in thinking about this more, I know I suggested making the
> KERNEL_IMAGE_SIZE by default 1G in order to simplify things.
> 
> But you're adding another KERNEL_MAPPING_SIZE which confuses things
> more. And I fail to see why we absolutely need it.

OK, I am trying to make things clearer, seems I failed. I thought kernel
iamge size is only allowed to be 512M at most, but can be mapped into 1G
region.
> 
> So we suggest kernel image size should be 512M but then we still will
> be using a whole 1G mapping for it anyway and a whole page of PMDs at
> level2_kernel_pgt.
> 
> So why even bother?
> 
> Just make it 1G and don't introduce anything new.

It's fine to me, thing can be solved anyway. Will repost with
KERNEL_IMAGE_SIZE by default 1G.

Thanks
Baoquan

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Borislav Petkov]

On Fri, Mar 03, 2017 at 08:06:16PM +0800, Baoquan He wrote:
> OK, I am trying to make things clearer, seems I failed. I thought kernel
> iamge size is only allowed to be 512M at most, but can be mapped into 1G
> region.

It doesn't look like it. But we could be missing something. You could
try some git archeology to find out why the 512M limit. It could be "no
reason", it could be remnant from 32-bit, it could be anything...

There's the full git history here too:

https://git.kernel.org/cgit/linux/kernel/git/history/history.git

in case it helps.

> It's fine to me, thing can be solved anyway. Will repost with
> KERNEL_IMAGE_SIZE by default 1G.

I think you should slow down first and try to find out why the 512
default. Then we can talk about changes.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 01:16pm, Borislav Petkov wrote:
> On Fri, Mar 03, 2017 at 08:06:16PM +0800, Baoquan He wrote:
> > OK, I am trying to make things clearer, seems I failed. I thought kernel
> > iamge size is only allowed to be 512M at most, but can be mapped into 1G
> > region.
> 
> It doesn't look like it. But we could be missing something. You could
> try some git archeology to find out why the 512M limit. It could be "no
> reason", it could be remnant from 32-bit, it could be anything...
> 
> There's the full git history here too:
> 
> https://git.kernel.org/cgit/linux/kernel/git/history/history.git
> 
> in case it helps.

Thanks, have got all related change history below. In the last commit log
Ingo wrote he was just trying to give more space to kernel and push
modules up a bit, and it should be enough for a few years. My thought of
introducing KERNEL_MAPPING_SIZE is mainly because in commit 85eb69a1
("x86: increase the kernel text limit to 512 MB") Ingo is trying to
increase kernel image size, since the really large static arrays and
building allyesconfig kernel are all bloating kernel image. I feel Ingo
is very careful to keep the pace to increase it, guess people don't want
to see kernel image can be made by default as 1G big at one time without
obvious reason. AFAIK, peopel sometime have to tell how much space it
will increae with their new feature or big change. This is a very good
self alert with a limited but usually enough value, 512M, let people pay
attention to the elegacy but not always many lines of code, think more
and refector code. And linker script is the guard to check it. 

Not sure if I make myself clear. I hesitated to do that earlier, so
finally introduce KERNEL_MAPPING_SIZE. Surely in the current case, as
you said, 1G is hard-constrainted line, unless we decide to give a
larger space for kernel mapping or put it other place since Intel people
have been working on 5-level page mapping thing, we don't lack virtual
space, but kernel mapping is too constrainted. Even though we have more
than 1G kernel mapping space, image size still should be limited to a
small value, like plus 128M or + 256M.

***
commit 85eb69a16aab5a394ce043c2131319eae35e6493
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Feb 21 12:50:51 2008 +0100

    x86: increase the kernel text limit to 512 MB
    
    people sometimes do crazy stuff like building really large static
    arrays into their kernels or building allyesconfig kernels. Give
    more space to the kernel and push modules up a bit: kernel has
    512 MB and modules have 1.5 GB.
    
    Should be enough for a few years ;-)

***
d4afe414    x86: rename KERNEL_TEXT_SIZE => KERNEL_IMAGE_SIZE
Rename it to be KERNEL_IMAGE_SIZE

***
88f3aec7    x86: fix spontaneous reboot with allyesconfig bzImage
In this commit Ingo changed KERNEL_TEXT_SIZE from 40M to 128M.

> 
> > It's fine to me, thing can be solved anyway. Will repost with
> > KERNEL_IMAGE_SIZE by default 1G.
> 
> I think you should slow down first and try to find out why the 512
> default. Then we can talk about changes.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> -- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 08:52pm, Baoquan He wrote:
> On 03/03/17 at 01:16pm, Borislav Petkov wrote:
> > On Fri, Mar 03, 2017 at 08:06:16PM +0800, Baoquan He wrote:
> > > OK, I am trying to make things clearer, seems I failed. I thought kernel
> > > iamge size is only allowed to be 512M at most, but can be mapped into 1G
> > > region.
> > 
> > It doesn't look like it. But we could be missing something. You could
> > try some git archeology to find out why the 512M limit. It could be "no
> > reason", it could be remnant from 32-bit, it could be anything...
> > 
> > There's the full git history here too:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/history/history.git
> > 
> > in case it helps.
> 

And another meaning of defining kernel iamge size and mapping size
differently is we can randomize the limited kernel image in the mapping
area. If they are the same or kernel image can be very large, the
position will be fixed or very few, kernel text KASLR will be
meaningless. E.g 512M of kernel image, 16M aligned, there are 32 slots
we can choose to position. If kernel image can be 1g either, no
possibility to randomize at all.

> Thanks, have got all related change history below. In the last commit log
> Ingo wrote he was just trying to give more space to kernel and push
> modules up a bit, and it should be enough for a few years. My thought of
> introducing KERNEL_MAPPING_SIZE is mainly because in commit 85eb69a1
> ("x86: increase the kernel text limit to 512 MB") Ingo is trying to
> increase kernel image size, since the really large static arrays and
> building allyesconfig kernel are all bloating kernel image. I feel Ingo
> is very careful to keep the pace to increase it, guess people don't want
> to see kernel image can be made by default as 1G big at one time without
> obvious reason. AFAIK, peopel sometime have to tell how much space it
> will increae with their new feature or big change. This is a very good
> self alert with a limited but usually enough value, 512M, let people pay
> attention to the elegacy but not always many lines of code, think more
> and refector code. And linker script is the guard to check it. 
> 
> Not sure if I make myself clear. I hesitated to do that earlier, so
> finally introduce KERNEL_MAPPING_SIZE. Surely in the current case, as
> you said, 1G is hard-constrainted line, unless we decide to give a
> larger space for kernel mapping or put it other place since Intel people
> have been working on 5-level page mapping thing, we don't lack virtual
> space, but kernel mapping is too constrainted. Even though we have more
> than 1G kernel mapping space, image size still should be limited to a
> small value, like plus 128M or + 256M.
> 
> ***
> commit 85eb69a16aab5a394ce043c2131319eae35e6493
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Thu Feb 21 12:50:51 2008 +0100
> 
>     x86: increase the kernel text limit to 512 MB
>     
>     people sometimes do crazy stuff like building really large static
>     arrays into their kernels or building allyesconfig kernels. Give
>     more space to the kernel and push modules up a bit: kernel has
>     512 MB and modules have 1.5 GB.
>     
>     Should be enough for a few years ;-)
> 
> ***
> d4afe414    x86: rename KERNEL_TEXT_SIZE => KERNEL_IMAGE_SIZE
> Rename it to be KERNEL_IMAGE_SIZE
> 
> ***
> 88f3aec7    x86: fix spontaneous reboot with allyesconfig bzImage
> In this commit Ingo changed KERNEL_TEXT_SIZE from 40M to 128M.
> 
> > 
> > > It's fine to me, thing can be solved anyway. Will repost with
> > > KERNEL_IMAGE_SIZE by default 1G.
> > 
> > I think you should slow down first and try to find out why the 512
> > default. Then we can talk about changes.
> > 
> > -- 
> > Regards/Gruss,
> >     Boris.
> > 
> > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> > -- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Borislav Petkov]

On Fri, Mar 03, 2017 at 09:11:52PM +0800, Baoquan He wrote:
> And another meaning of defining kernel iamge size and mapping size
> differently is we can randomize the limited kernel image in the mapping
> area. If they are the same or kernel image can be very large, the
> position will be fixed or very few, kernel text KASLR will be
> meaningless.

This is simply not true:

@@ -408,9 +408,9 @@ static unsigned long find_random_virt_addr(unsigned long minimum,
        /*
         * There are how many CONFIG_PHYSICAL_ALIGN-sized slots
         * that can hold image_size within the range of minimum to
-        * KERNEL_IMAGE_SIZE?
+        * KERNEL_MAPPING_SIZE?
         */
-       slots = (KERNEL_IMAGE_SIZE - minimum - image_size) /
+       slots = (KERNEL_MAPPING_SIZE - minimum - image_size) /
                 CONFIG_PHYSICAL_ALIGN + 1;

*With* kaslr, KERNEL_IMAGE_SIZE = 1G and KERNEL_MAPPING_SIZE = 1G.
Before your patch KERNEL_IMAGE_SIZE = 1G too with kaslr enabled.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 03:28pm, Borislav Petkov wrote:
> On Fri, Mar 03, 2017 at 09:11:52PM +0800, Baoquan He wrote:
> > And another meaning of defining kernel iamge size and mapping size
> > differently is we can randomize the limited kernel image in the mapping
> > area. If they are the same or kernel image can be very large, the
> > position will be fixed or very few, kernel text KASLR will be
> > meaningless.
> 
> This is simply not true:
> 
> @@ -408,9 +408,9 @@ static unsigned long find_random_virt_addr(unsigned long minimum,
>         /*
>          * There are how many CONFIG_PHYSICAL_ALIGN-sized slots
>          * that can hold image_size within the range of minimum to
> -        * KERNEL_IMAGE_SIZE?
> +        * KERNEL_MAPPING_SIZE?
>          */
> -       slots = (KERNEL_IMAGE_SIZE - minimum - image_size) /
> +       slots = (KERNEL_MAPPING_SIZE - minimum - image_size) /
>                  CONFIG_PHYSICAL_ALIGN + 1;
> 
> *With* kaslr, KERNEL_IMAGE_SIZE = 1G and KERNEL_MAPPING_SIZE = 1G.
> Before your patch KERNEL_IMAGE_SIZE = 1G too with kaslr enabled.

512M and 1G is the first case, just an example. Usually kernel image size
is only about 20M, from my laptop.

Yes, before KERNEL_IMAGE_SIZE is 1G with kaslr enabled. when you
suggested taking a fixed size for the KERNEL_IMAGE_SIZE, but not changed
back and forth with the kaslr set or not, I started to consider this.

See the 1G, hard-constrainted because of level2_kernel_pgt. In the
future we could put kernel mapping area in another place to remove the
1G limitation, could be 10G or 512G since virtual address are so
redundent, just an assumption, kernel KASLR can benefit from this
actually, but we can't make upper value of kernel image size also be
that big. That will make linker script checking lose meaning.

Thanks
Baoquan

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 11:07pm, Baoquan He wrote:
> On 03/03/17 at 03:28pm, Borislav Petkov wrote:
> > On Fri, Mar 03, 2017 at 09:11:52PM +0800, Baoquan He wrote:
> > > And another meaning of defining kernel iamge size and mapping size
> > > differently is we can randomize the limited kernel image in the mapping
> > > area. If they are the same or kernel image can be very large, the
> > > position will be fixed or very few, kernel text KASLR will be
> > > meaningless.
> > 
> > This is simply not true:
> > 
> > @@ -408,9 +408,9 @@ static unsigned long find_random_virt_addr(unsigned long minimum,
> >         /*
> >          * There are how many CONFIG_PHYSICAL_ALIGN-sized slots
> >          * that can hold image_size within the range of minimum to
> > -        * KERNEL_IMAGE_SIZE?
> > +        * KERNEL_MAPPING_SIZE?
> >          */
> > -       slots = (KERNEL_IMAGE_SIZE - minimum - image_size) /
> > +       slots = (KERNEL_MAPPING_SIZE - minimum - image_size) /
> >                  CONFIG_PHYSICAL_ALIGN + 1;
> > 
> > *With* kaslr, KERNEL_IMAGE_SIZE = 1G and KERNEL_MAPPING_SIZE = 1G.
> > Before your patch KERNEL_IMAGE_SIZE = 1G too with kaslr enabled.
> 
> 512M and 1G is the first case, just an example. Usually kernel image size
			~ worst, sorry, typo
> is only about 20M, from my laptop.
> 
> Yes, before KERNEL_IMAGE_SIZE is 1G with kaslr enabled. when you
> suggested taking a fixed size for the KERNEL_IMAGE_SIZE, but not changed
> back and forth with the kaslr set or not, I started to consider this.
> 
> See the 1G, hard-constrainted because of level2_kernel_pgt. In the
> future we could put kernel mapping area in another place to remove the
> 1G limitation, could be 10G or 512G since virtual address are so
> redundent, just an assumption, kernel KASLR can benefit from this
> actually, but we can't make upper value of kernel image size also be
> that big. That will make linker script checking lose meaning.
> 
> Thanks
> Baoquan
> 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Borislav Petkov]

Ok,

TBH, I still don't like adding yet another define and paying attention
to whether I should use image size or mapping size. After your patch,
KERNEL_IMAGE_SIZE is used to enforce the actual image size from
exploding:

arch/x86/include/asm/page_32_types.h:43:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
arch/x86/include/asm/page_32_types.h:49:#define KERNEL_MAPPING_SIZE     KERNEL_IMAGE_SIZE
arch/x86/include/asm/page_64_types.h:56:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
arch/x86/include/asm/pgtable_32.h:83: *     (KERNEL_IMAGE_SIZE/4096) / 1024 pages (worst case, non PAE)
arch/x86/include/asm/pgtable_32.h:84: *     (KERNEL_IMAGE_SIZE/4096) / 512 + 4 pages (worst case for PAE)
arch/x86/include/asm/pgtable_32.h:91: * KERNEL_IMAGE_SIZE should be greater than pa(_end)
arch/x86/kernel/vmlinux.lds.S:356:. = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
arch/x86/kernel/vmlinux.lds.S:357:         "kernel image bigger than KERNEL_IMAGE_SIZE");
arch/x86/kernel/vmlinux.lds.S:370:. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
arch/x86/kernel/vmlinux.lds.S:371:         "kernel image bigger than KERNEL_IMAGE_SIZE");

So what I'd do is keep KERNEL_IMAGE_SIZE and make it default 1G and use it
everywhere.

Then, define a separate define which is used only in vmlinux.lds.S to
enforce the size check. Having MAPPING_SIZE and IMAGE_SIZE is just
needlessly confusing.

Especially if this is just some inconsistency you're addressing and not
some real issue.

BUT(!), don't take my word for it. Rather, do what the maintainers
propose. Who knows, they might have a much better idea.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 04:23pm, Borislav Petkov wrote:
> Ok,
> 
> TBH, I still don't like adding yet another define and paying attention
> to whether I should use image size or mapping size. After your patch,
> KERNEL_IMAGE_SIZE is used to enforce the actual image size from
> exploding:
> 
> arch/x86/include/asm/page_32_types.h:43:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
> arch/x86/include/asm/page_32_types.h:49:#define KERNEL_MAPPING_SIZE     KERNEL_IMAGE_SIZE
> arch/x86/include/asm/page_64_types.h:56:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
> arch/x86/include/asm/pgtable_32.h:83: *     (KERNEL_IMAGE_SIZE/4096) / 1024 pages (worst case, non PAE)
> arch/x86/include/asm/pgtable_32.h:84: *     (KERNEL_IMAGE_SIZE/4096) / 512 + 4 pages (worst case for PAE)
> arch/x86/include/asm/pgtable_32.h:91: * KERNEL_IMAGE_SIZE should be greater than pa(_end)
> arch/x86/kernel/vmlinux.lds.S:356:. = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
> arch/x86/kernel/vmlinux.lds.S:357:         "kernel image bigger than KERNEL_IMAGE_SIZE");
> arch/x86/kernel/vmlinux.lds.S:370:. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
> arch/x86/kernel/vmlinux.lds.S:371:         "kernel image bigger than KERNEL_IMAGE_SIZE");
> 
> So what I'd do is keep KERNEL_IMAGE_SIZE and make it default 1G and use it
> everywhere.
> 
> Then, define a separate define which is used only in vmlinux.lds.S to
> enforce the size check. Having MAPPING_SIZE and IMAGE_SIZE is just
> needlessly confusing.

Yes, in fact if only look at 64 bit, what I am doing is just as you
suggested. KERNEL_IMAGE_SIZE is only used to limit the image size,
namely _end - _text.

. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
           "kernel image bigger than KERNEL_IMAGE_SIZE");

And I could get what's confusing, should be the name of
KERNEL_MAPPING_SIZE. If only talking about the kernel itself, the
mapping size needed to cover the whole kernel iamge should be the same
as kernel image size. Say kernel image size ( _end - _text) is 86M, then
if kaslr-ed to a postion at 0xffffffffa0000000, which is 512M above the
starting address of virtual address space used for kernel mapping, the
kernel iamge will be mapped at region:
	[0xffffffffa0000000, 0xffffffffa5600000]

So here KERNEL_MAPPING_SIZE is not meaning the size of the mapping
region of kernel image itself, but the whole virtual address space which
is available only for kernel mapping, nobody else can touch this area.
It's 1G big, and cover region [0xffffffff80000000, 0xffffffffbfffffff].
It might be named as SIZE_OF_SPACE_FOR_KERNEL_MAPPING, but that is not a
good name, so I just named as KERNEL_MAPPING_SIZE. It could be
confusing.

> 
> Especially if this is just some inconsistency you're addressing and not
> some real issue.

I think it's not simply fixing inconsistency thing now, according to a
lot of discussion, we all agree that there's no need to change the size
of space for kernel mapping back and forth, 512M to 1G, 1G back to 512M,
risk isn't felt when shrink kernel modules space to 1G constantly.

> 
> BUT(!), don't take my word for it. Rather, do what the maintainers
> propose. Who knows, they might have a much better idea.

Sorry about that. Just think your words are very convincing on removing
people's doubt if it's risky to shrink kernel modules space to 1G. Will
remove the words mentioning you said it since you don't like it. Didn't
realize that, no offence.

Thanks
Baoquan

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Borislav Petkov]

On Sat, Mar 04, 2017 at 06:10:37PM +0800, Baoquan He wrote:
> > BUT(!), don't take my word for it. Rather, do what the maintainers
> > propose. Who knows, they might have a much better idea.
> 
> Sorry about that. Just think your words are very convincing on removing
> people's doubt if it's risky to shrink kernel modules space to 1G. Will
> remove the words mentioning you said it since you don't like it. Didn't
> realize that, no offence.

No, this is not what I mean at all!

I'm saying, I tried to review your patches and I don't like the end
result because it adds more complexity. And the reason(s) for it are not
persuading me enough to make me say: "yeah, this is a good thing, I want
it."

But this is only my opinion. That's all. The final decision is in the
hands of the x86 maintainers.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/04/17 at 12:55pm, Borislav Petkov wrote:
> On Sat, Mar 04, 2017 at 06:10:37PM +0800, Baoquan He wrote:
> > > BUT(!), don't take my word for it. Rather, do what the maintainers
> > > propose. Who knows, they might have a much better idea.
> > 
> > Sorry about that. Just think your words are very convincing on removing
> > people's doubt if it's risky to shrink kernel modules space to 1G. Will
> > remove the words mentioning you said it since you don't like it. Didn't
> > realize that, no offence.
> 
> No, this is not what I mean at all!
> 
> I'm saying, I tried to review your patches and I don't like the end
> result because it adds more complexity. And the reason(s) for it are not
> persuading me enough to make me say: "yeah, this is a good thing, I want
> it."
> 
> But this is only my opinion. That's all. The final decision is in the
> hands of the x86 maintainers.

Got it, sorry for the misunderstanding. I really appreciate your
reviewing, great comments and suggestions. Glad to see that now we
don't hesitate to shrink kernel modules area to 1G after discussion. I
will ping Ingo to ask if he has any suggestion since he has been taking
care of the KERNEL_IMAGE_SIZE value changing.

Thanks
Baoquan

Re: [PATCH v4 0/3] x86/64/KASLR: Change kernel mapping size to 1G unconditionally

[Baoquan He]

Hi Ingo,

Ping!

Could you please help have a look at thie patchset and give comments or
any suggestion?

Checked the git history, you have been taking care of the
KERNEL_IMAGE_SIZE value changing. Now Kees added his Reviewed-by, Boris 
agreed on setting the size of space for kernel mapping to 1G by default
but he doesn't like the adding KERNEL_MAPPING_SIZE constant idea.

Your suggestion is much appreciated!

Thanks
Baoquan

On 02/02/17 at 08:54pm, Baoquan He wrote:
> This is v4 post.
> 
> In the previous 3 versions I tried to detect and determine kernel image
> mapping size at runtime for x86_64. With this the inconsistency between
> KASLR code is not compiled in by disabling CONFIG_RANDOMIZE_BASE and
> KASLR code is compiled in but add 'nokaslr' kernel option can be fixed.
> 
> When review v3 Boris suggested we should change kernel mapping size to
> 1G directly, but not an option. Kees explained he made kernel mapping
> size as an option mainly because he woried about kernel module space.
> He said it wasn't clear to him at the time if enough space remained for
> modules in all use-cases. Then Boris pointed out that practically kaslr
> will be enabled on the majority of the systems anyway, and the reduction
> of kernel modules mapping space has been there for a while now, if so we
> probably whould've heard complaints already. Kees agreed.
> 
> So in this version of post change kernel mapping size of x86 64 to 1G
> as Boris suggested. Then the inconsistency will naturally disappear.
> 
> And I still keep patch 1/3 which introduces the new constant
> KERNEL_MAPPING_SIZE. And let KERNEL_IMAGE_SIZE stay for restricting kernel
> image size during linking stage.
> 
> 
> v3->v4:
>     Change kernel mapping size to 1G unconditionally as Boris suggested.
> 
> v2->v3:
>     Boris pointed out patch log is not good for reviewing and understanding.
>     So split the old patch 2/2 into 2 parts and rewrite the patch log,
>     patch 2/3 is introducing the new constant KERNEL_MAPPING_SIZE which
>     differs from the old KERNEL_IMAGE_SIZE, patch 3/3 gets the kernel mapping
>     size at runtime.
> 
> v1->v2:
>     Kbuild test threw build warning because of code bug.
> 
> 
> Baoquan He (3):
>   x86: Introduce a new constant KERNEL_MAPPING_SIZE
>   x86/64/KASLR: Change kernel mapping size to 1G unconditionally
>   x86/64/doc: Update the ranges of kernel text and modules mapping
> 
>  Documentation/x86/x86_64/mm.txt         |  4 ++--
>  arch/x86/boot/compressed/kaslr.c        | 10 +++++-----
>  arch/x86/include/asm/page_32_types.h    |  6 ++++++
>  arch/x86/include/asm/page_64_types.h    | 17 ++++++++---------
>  arch/x86/include/asm/pgtable_64_types.h |  2 +-
>  arch/x86/kernel/head64.c                |  4 ++--
>  arch/x86/kernel/head_64.S               | 12 +++++-------
>  arch/x86/kernel/machine_kexec_64.c      |  2 +-
>  arch/x86/mm/init_64.c                   |  2 +-
>  arch/x86/mm/physaddr.c                  |  6 +++---
>  10 files changed, 34 insertions(+), 31 deletions(-)
> 
> -- 
> 2.5.5
> 

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Ingo Molnar]

* Borislav Petkov <bp@suse.de> wrote:

> Ok,
> 
> TBH, I still don't like adding yet another define and paying attention
> to whether I should use image size or mapping size. After your patch,
> KERNEL_IMAGE_SIZE is used to enforce the actual image size from
> exploding:
> 
> arch/x86/include/asm/page_32_types.h:43:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
> arch/x86/include/asm/page_32_types.h:49:#define KERNEL_MAPPING_SIZE     KERNEL_IMAGE_SIZE
> arch/x86/include/asm/page_64_types.h:56:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
> arch/x86/include/asm/pgtable_32.h:83: *     (KERNEL_IMAGE_SIZE/4096) / 1024 pages (worst case, non PAE)
> arch/x86/include/asm/pgtable_32.h:84: *     (KERNEL_IMAGE_SIZE/4096) / 512 + 4 pages (worst case for PAE)
> arch/x86/include/asm/pgtable_32.h:91: * KERNEL_IMAGE_SIZE should be greater than pa(_end)
> arch/x86/kernel/vmlinux.lds.S:356:. = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
> arch/x86/kernel/vmlinux.lds.S:357:         "kernel image bigger than KERNEL_IMAGE_SIZE");
> arch/x86/kernel/vmlinux.lds.S:370:. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
> arch/x86/kernel/vmlinux.lds.S:371:         "kernel image bigger than KERNEL_IMAGE_SIZE");
> 
> So what I'd do is keep KERNEL_IMAGE_SIZE and make it default 1G and use it
> everywhere.
> 
> Then, define a separate define which is used only in vmlinux.lds.S to
> enforce the size check. Having MAPPING_SIZE and IMAGE_SIZE is just
> needlessly confusing.

That sounds like the right solution to me - having two values is asking for 
trouble.

Thanks,

	Ingo

Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/16/17 at 09:14am, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@suse.de> wrote:
> > So what I'd do is keep KERNEL_IMAGE_SIZE and make it default 1G and use it
> > everywhere.
> > 
> > Then, define a separate define which is used only in vmlinux.lds.S to
> > enforce the size check. Having MAPPING_SIZE and IMAGE_SIZE is just
> > needlessly confusing.
> 
> That sounds like the right solution to me - having two values is asking for 
> trouble.

Thanks for your suggestion.

OK, I will repost with only changing KERNEL_IMAGE_SIZE to 1G, just like
CONFIG_RANDOMIZE_BASE is enabled in old code.

I made one but haven't tested it yet, do you think it's OK?


>From eb8ab3e0c1cbe364dbd1d59cc1875a2728df700c Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Thu, 16 Mar 2017 16:36:33 +0800
Subject: [PATCH] x86/64/KASLR: Change KERNEL_IMAGE_SIZE to 1G unconditionally

The current KASLR changes KERNEL_IMAGE_SIZE from 512M to 1G as long
as CONFIG_RANDOMIZE_BASE is enabled, though "nokaslr" kernel option is
added. This is buggy. When people specify "nokaslr", whether KASLR code
compiled in or not, they expect to see no KASLR change at all, including
the default limit size of kernel image and size of module space.

Kees explained the only reason he made KERNEL_IMAGE_SIZE as an option
was for kernel module space. It wasn't clear at the time if enough space
remained for modules in all use-cases.

Boris suggested we can make KERNEL_IMAGE_SIZE 1G unconditionally since
practically kaslr will be enabled on the majority of the systems anyway,
so we will have 1G KERNEL_IMAGE_SIZE on most. And he further pointed out
that: [Quote his words as follows]
"""""
Realistically, on a typical bigger machine, the modules take up
something like <10M:

$ lsmod | awk '{ sum +=$2 } END { print sum }'
7188480

so I'm not really worried if we reduce it by default to 1G. Besides, the
reduction has been there for a while now - since CONFIG_RANDOMIZE_BASE -
so we probably would've heard complaints already...
"""""

Hence in this patch change KERNEL_IMAGE_SIZE to 1G as suggested.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/include/asm/page_64_types.h | 10 ++--------
 arch/x86/kernel/head_64.S            |  5 ++---
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 9215e05..98bf5a0 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -51,15 +51,9 @@
 
 /*
  * Kernel image size is limited to 1GiB due to the fixmap living in the
- * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Use
- * 512MiB by default, leaving 1.5GiB for modules once the page tables
- * are fully set up. If kernel ASLR is configured, it can extend the
- * kernel page table mapping, reducing the size of the modules area.
+ * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Leaving
+ * 1GiB for modules once the page tables are fully set up.
  */
-#if defined(CONFIG_RANDOMIZE_BASE)
 #define KERNEL_IMAGE_SIZE	(1024 * 1024 * 1024)
-#else
-#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
-#endif
 
 #endif /* _ASM_X86_PAGE_64_DEFS_H */
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index b467b14..1e98617 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -458,12 +458,11 @@ NEXT_PAGE(level3_kernel_pgt)
 
 NEXT_PAGE(level2_kernel_pgt)
 	/*
-	 * 512 MB kernel mapping. We spend a full page on this pagetable
-	 * anyway.
+	 * 1GiB kernel mapping. We spend a full page on this pagetable.
 	 *
 	 * The kernel code+data+bss must not be bigger than that.
 	 *
-	 * (NOTE: at +512MB starts the module area, see MODULES_VADDR.
+	 * (NOTE: at +1GiB starts the module area, see MODULES_VADDR.
 	 *  If you want to increase this then increase MODULES_VADDR
 	 *  too.)
 	 */
-- 
2.5.5