[PATCH v3 0/4] kaslr: add new parameter immovable_mem=nn[KMG]@ss[KMG] to make memory hotplug work well with kaslr

[PATCH v3 0/4] kaslr: add new parameter immovable_mem=nn[KMG]@ss[KMG] to make memory hotplug work well with kaslr

[Chao Fan]

Here is a problem:
Here is a machine with several NUMA nodes and some of them are
hot-pluggable. It's not good for kernel to be extracted in the memory
region of movable node. But in current code, I print the address chosen by
kaslr and found it may be placed in movable node sometimes.
To solve this problem, it's better to limit the memory region chosen by
kaslr to immovable node in kaslr.c. But the memory information about if
it's hot-pluggable is stored in ACPI SRAT table, which is parsed after
kernel is extracted. So we can't get the detail memory information
before extracting kernel.

So add the new parameter immovable_mem=nn@ss, in which nn means
the size of memory in *immovable* node, and ss means the start position of
this memory region. Then limit kaslr choose memory in these regions.

There are two policies:
1. Specify the memory region in *movable* node to avoid:
   Then we can use the existing mem_avoid to handle. But if the memory
   on movable node was separated by memory hole or different movable nodes
   are discontinuous, we don't know how many regions need to avoid.
   OTOH, we must avoid all of the movable memory, otherwise, kaslr may
   choose the wrong place.
2. Specify the memory region in *immovable* node to select:
   Only support 4 regions in this parameter. Then user can use two nodes
   at least for kaslr to choose, it's enough for the kernel to extract.
   At the same time, because we need only 4 new mem_vector, the usage
   of memory here is not too big. So I think this way is better, and this 
   patchset is based on this policy.

PATCH 1/4 parse the new parameter immovable_mem=nn[KMG]@ss[KMG], then
          store the memory regions.
PATCH 2/4 select the memory region in immovable node when process
          memmap.
PATCH 3/4 skip mirror feature if movable_node or immovable_mem specified.
PATCH 4/4 add document.

v1->v2:
Follow Dou Liyang's suggestion:
 - Add the parse for movable_node=nn[KMG] without @ss[KMG]
 - Fix the bug for more than one "movable_node=" specified
 - Drop useless variables and use mem_vector region directely
 - Add more comments.

v2->v3:
Follow Baoquan He's suggestion:
 - Change names of several functions.
 - Add a new parameter "immovable_mem" instead of extending mvoable_node
 - Use the clamp to calculate the memory intersecting, which makes
   logical more clear.
 - Disable memory mirror if movable_node specified


Chao Fan (4):
  kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory
  kaslr: calculate the memory region in immovable node
  kaslr: disable memory mirror feature when movable_node specified
  document: change the document for immovable_mem

 Documentation/admin-guide/kernel-parameters.txt |   9 ++
 arch/x86/boot/compressed/kaslr.c                | 154 +++++++++++++++++++++---
 2 files changed, 147 insertions(+), 16 deletions(-)

-- 
2.14.3

[PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Chao Fan]

In current code, kaslr may choose the memory region in movable
nodes to extract kernel, which will make the nodes can't be hot-removed.
To solve it, we can specify the memory region in immovable node.
Create immovable_mem to store the regions in immovable_mem, where should
be chosen by kaslr.

Multiple regions can be specified, comma delimited.
Considering the usage of memory, only support for 4 regions.
4 regions contains 2 nodes at least, enough for kernel to extract.

Also change the "handle_mem_memmap" to "handle_mem_filter", since
it will not only handle memmap parameter now.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index a63fbc25ce84..0bbbaf5f6370 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -108,6 +108,15 @@ enum mem_avoid_index {
 
 static struct mem_vector mem_avoid[MEM_AVOID_MAX];
 
+/* Only supporting at most 4 immovable memory regions with kaslr */
+#define MAX_IMMOVABLE_MEM	4
+
+/* Store the memory regions in immovable node */
+static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
+
+/* The immovable regions user specify, not more than 4 */
+static int num_immovable_region;
+
 static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
 {
 	/* Item one is entirely before item two. */
@@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 	return -EINVAL;
 }
 
+static int parse_immovable_mem(char *p,
+			       unsigned long long *start,
+			       unsigned long long *size)
+{
+	char *oldp;
+
+	if (!p)
+		return -EINVAL;
+
+	oldp = p;
+	*size = memparse(p, &p);
+	if (p == oldp)
+		return -EINVAL;
+
+	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
+	switch (*p) {
+	case '@':
+		*start = memparse(p + 1, &p);
+		return 0;
+	default:
+		/*
+		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
+		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
+		 * the region starts from 0.
+		 */
+		*start = 0;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static void mem_avoid_memmap(char *str)
 {
 	static int i;
@@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
 		memmap_too_large = true;
 }
 
-static int handle_mem_memmap(void)
+#ifdef CONFIG_MEMORY_HOTPLUG
+static void parse_immovable_mem_regions(char *str)
+{
+	static int i;
+
+	while (str && (i < MAX_IMMOVABLE_MEM)) {
+		int rc;
+		unsigned long long start, size;
+		char *k = strchr(str, ',');
+
+		if (k)
+			*k++ = 0;
+
+		rc = parse_immovable_mem(str, &start, &size);
+		if (rc < 0)
+			break;
+		str = k;
+
+		immovable_mem[i].start = start;
+		immovable_mem[i].size = size;
+		i++;
+	}
+	num_immovable_region = i;
+}
+#else
+static inline void parse_immovable_mem_regions(char *str)
+{
+}
+#endif
+
+static int handle_mem_filter(void)
 {
 	char *args = (char *)get_cmd_line_ptr();
 	size_t len = strlen((char *)args);
@@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
 	char *param, *val;
 	u64 mem_size;
 
-	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
+	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
+	    !strstr(args, "immovable_mem="))
 		return 0;
 
 	tmp_cmdline = malloc(len + 1);
@@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
 
 		if (!strcmp(param, "memmap")) {
 			mem_avoid_memmap(val);
+		} else if (!strcmp(param, "immovable_mem")) {
+			parse_immovable_mem_regions(val);
 		} else if (!strcmp(param, "mem")) {
 			char *p = val;
 
@@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* We don't need to set a mapping for setup_data. */
 
 	/* Mark the memmap regions we need to avoid */
-	handle_mem_memmap();
+	handle_mem_filter();
 
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
-- 
2.14.3

[PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Chao Fan]

If there is no immovable memory region specified, go on the old code.
There are several conditons:
1. CONFIG_MEMORY_HOTPLUG is not specified to y.
2. immovable_mem= is not specified.

Otherwise, calculate the intersecting between memmap entry and
immovable memory.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 59 ++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 8 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 0bbbaf5f6370..e3a3b6132da0 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -635,6 +635,39 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+static bool process_immovable_mem(struct mem_vector region,
+				  unsigned long long minimum,
+				  unsigned long long image_size)
+{
+	int i;
+
+	/*
+	 * Walk all immovable regions, and filter the intersection
+	 * to process_mem_region.
+	 */
+	for (i = 0; i < num_immovable_region; i++) {
+		struct mem_vector entry;
+		unsigned long long start, end, entry_end;
+
+		start = immovable_mem[i].start;
+		end = start + immovable_mem[i].size;
+
+		entry.start = clamp(region.start, start, end);
+		entry_end = clamp(region.start + region.size, start, end);
+
+		if (entry.start < entry_end) {
+			entry.size = entry_end - entry.start;
+			process_mem_region(&entry, minimum, image_size);
+		}
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+	}
+	return 0;
+}
+
 #ifdef CONFIG_EFI
 /*
  * Returns true if mirror region found (and must have been processed
@@ -700,11 +733,16 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 
 		region.start = md->phys_addr;
 		region.size = md->num_pages << EFI_PAGE_SHIFT;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+
+		/* If no immovable_mem stored, use region directly */
+		if (num_immovable_region == 0) {
+			process_mem_region(&region, minimum, image_size);
+			if (slot_area_index == MAX_SLOT_AREA) {
+				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
+				break;
+			}
+		} else if (process_immovable_mem(region, minimum, image_size))
 			break;
-		}
 	}
 	return true;
 }
@@ -731,11 +769,16 @@ static void process_e820_entries(unsigned long minimum,
 			continue;
 		region.start = entry->addr;
 		region.size = entry->size;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+
+		/* If no immovable_mem stored, use region directly */
+		if (num_immovable_region == 0) {
+			process_mem_region(&region, minimum, image_size);
+			if (slot_area_index == MAX_SLOT_AREA) {
+				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
+				break;
+			}
+		} else if (process_immovable_mem(region, minimum, image_size))
 			break;
-		}
 	}
 }
 
-- 
2.14.3

[PATCH v3 3/4] kaslr: disable memory mirror feature when movable_node specified

[Chao Fan]

In kernel code, if movable_node specified, it will skip the mirror
feature. So we should also skip mirror feature in kaslr.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index e3a3b6132da0..fc798d6d608d 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -677,6 +677,7 @@ static bool
 process_efi_entries(unsigned long minimum, unsigned long image_size)
 {
 	struct efi_info *e = &boot_params->efi_info;
+	char *args = (char *)get_cmd_line_ptr();
 	bool efi_mirror_found = false;
 	struct mem_vector region;
 	efi_memory_desc_t *md;
@@ -702,11 +703,15 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 #endif
 
 	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
-	for (i = 0; i < nr_desc; i++) {
-		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
-		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
-			efi_mirror_found = true;
-			break;
+
+	/* Skip memory mirror if movabale_node or immovable_mem specified */
+	if (!strstr(args, "movable_node") && !strstr(args, "immovable_mem")) {
+		for (i = 0; i < nr_desc; i++) {
+			md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
+			if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
+				efi_mirror_found = true;
+				break;
+			}
 		}
 	}
 
-- 
2.14.3

[PATCH v3 4/4] document: change the document for immovable_mem

[Chao Fan]

Add the document for the change of new parameter
immovable_mem=nn[KMG]@ss[KMG].

Cc: linux-doc@vger.kernel.org
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b74e13312fdc..eea755af697f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2355,6 +2355,15 @@
 			allocations which rules out almost all kernel
 			allocations. Use with caution!
 
+	immovable_mem=nn[KMG]@ss[KMG]
+			[KNL] Force usage of a specific region of memory.
+			Make memory hotplug  work well with KASLR.
+			Region of memory in immovable node is from ss to ss+nn.
+			Multiple regions can be specified, comma delimited.
+			Notice: we support 4 regions at most now.
+			Example:
+			immovable_mem=1G,500M@2G,1G@4G
+
 	MTD_Partition=	[MTD]
 			Format: <name>,<region-number>,<size>,<offset>
 
-- 
2.14.3

Re: [PATCH v3 4/4] document: change the document for immovable_mem

[Randy Dunlap]

On 12/05/2017 12:52 AM, Chao Fan wrote:
> Add the document for the change of new parameter
> immovable_mem=nn[KMG]@ss[KMG].
> 
> Cc: linux-doc@vger.kernel.org
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b74e13312fdc..eea755af697f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2355,6 +2355,15 @@
>  			allocations which rules out almost all kernel
>  			allocations. Use with caution!
>  
> +	immovable_mem=nn[KMG]@ss[KMG]

	immovable_mem=nn[KMG][@ss[KMG]]

> +			[KNL] Force usage of a specific region of memory.
> +			Make memory hotplug  work well with KASLR.

			                   ^^only one space, please

> +			Region of memory in immovable node is from ss to ss+nn.> +			Multiple regions can be specified, comma delimited.
			If ss is omitted, it defaults to 0.
> +			Notice: we support 4 regions at most now.
> +			Example:
> +			immovable_mem=1G,500M@2G,1G@4G
> +
>  	MTD_Partition=	[MTD]
>  			Format: <name>,<region-number>,<size>,<offset>
>  
> 


-- 
~Randy

Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Kees Cook]

On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> If there is no immovable memory region specified, go on the old code.
> There are several conditons:
> 1. CONFIG_MEMORY_HOTPLUG is not specified to y.
> 2. immovable_mem= is not specified.
>
> Otherwise, calculate the intersecting between memmap entry and
> immovable memory.

Instead of copy/pasting code between process_efi_entries() and
process_e820_entries(), I'd rather that process_mem_region() is
modified to deal with immovable regions.

-Kees

>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 59 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 0bbbaf5f6370..e3a3b6132da0 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -635,6 +635,39 @@ static void process_mem_region(struct mem_vector *entry,
>         }
>  }
>
> +static bool process_immovable_mem(struct mem_vector region,
> +                                 unsigned long long minimum,
> +                                 unsigned long long image_size)
> +{
> +       int i;
> +
> +       /*
> +        * Walk all immovable regions, and filter the intersection
> +        * to process_mem_region.
> +        */
> +       for (i = 0; i < num_immovable_region; i++) {
> +               struct mem_vector entry;
> +               unsigned long long start, end, entry_end;
> +
> +               start = immovable_mem[i].start;
> +               end = start + immovable_mem[i].size;
> +
> +               entry.start = clamp(region.start, start, end);
> +               entry_end = clamp(region.start + region.size, start, end);
> +
> +               if (entry.start < entry_end) {
> +                       entry.size = entry_end - entry.start;
> +                       process_mem_region(&entry, minimum, image_size);
> +               }
> +
> +               if (slot_area_index == MAX_SLOT_AREA) {
> +                       debug_putstr("Aborted memmap scan (slot_areas full)!\n");
> +                       return 1;
> +               }
> +       }
> +       return 0;
> +}
> +
>  #ifdef CONFIG_EFI
>  /*
>   * Returns true if mirror region found (and must have been processed
> @@ -700,11 +733,16 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>
>                 region.start = md->phys_addr;
>                 region.size = md->num_pages << EFI_PAGE_SHIFT;
> -               process_mem_region(&region, minimum, image_size);
> -               if (slot_area_index == MAX_SLOT_AREA) {
> -                       debug_putstr("Aborted EFI scan (slot_areas full)!\n");
> +
> +               /* If no immovable_mem stored, use region directly */
> +               if (num_immovable_region == 0) {
> +                       process_mem_region(&region, minimum, image_size);
> +                       if (slot_area_index == MAX_SLOT_AREA) {
> +                               debug_putstr("Aborted memmap scan (slot_areas full)!\n");
> +                               break;
> +                       }
> +               } else if (process_immovable_mem(region, minimum, image_size))
>                         break;
> -               }
>         }
>         return true;
>  }
> @@ -731,11 +769,16 @@ static void process_e820_entries(unsigned long minimum,
>                         continue;
>                 region.start = entry->addr;
>                 region.size = entry->size;
> -               process_mem_region(&region, minimum, image_size);
> -               if (slot_area_index == MAX_SLOT_AREA) {
> -                       debug_putstr("Aborted e820 scan (slot_areas full)!\n");
> +
> +               /* If no immovable_mem stored, use region directly */
> +               if (num_immovable_region == 0) {
> +                       process_mem_region(&region, minimum, image_size);
> +                       if (slot_area_index == MAX_SLOT_AREA) {
> +                               debug_putstr("Aborted memmap scan (slot_areas full)!\n");
> +                               break;
> +                       }
> +               } else if (process_immovable_mem(region, minimum, image_size))
>                         break;
> -               }
>         }
>  }
>
> --
> 2.14.3
>
>
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Kees Cook]

On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> In current code, kaslr may choose the memory region in movable
> nodes to extract kernel, which will make the nodes can't be hot-removed.
> To solve it, we can specify the memory region in immovable node.
> Create immovable_mem to store the regions in immovable_mem, where should
> be chosen by kaslr.
>
> Multiple regions can be specified, comma delimited.
> Considering the usage of memory, only support for 4 regions.
> 4 regions contains 2 nodes at least, enough for kernel to extract.
>
> Also change the "handle_mem_memmap" to "handle_mem_filter", since
> it will not only handle memmap parameter now.

I would put all immovable region functions and variables behind
#ifdefs. e.g. parse_immovable_mem() is unused without
CONFIG_MEMORY_HOTPLUG. I also wonder if it might be possible to reuse
some of the parsing code and just pass in which array you're updating
(i.e. memmap array vs immovable array).

-Kees

>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index a63fbc25ce84..0bbbaf5f6370 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -108,6 +108,15 @@ enum mem_avoid_index {
>
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>
> +/* Only supporting at most 4 immovable memory regions with kaslr */
> +#define MAX_IMMOVABLE_MEM      4
> +
> +/* Store the memory regions in immovable node */
> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> +
> +/* The immovable regions user specify, not more than 4 */
> +static int num_immovable_region;
> +
>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>  {
>         /* Item one is entirely before item two. */
> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>         return -EINVAL;
>  }
>
> +static int parse_immovable_mem(char *p,
> +                              unsigned long long *start,
> +                              unsigned long long *size)
> +{
> +       char *oldp;
> +
> +       if (!p)
> +               return -EINVAL;
> +
> +       oldp = p;
> +       *size = memparse(p, &p);
> +       if (p == oldp)
> +               return -EINVAL;
> +
> +       /* We support nn[KMG]@ss[KMG] and nn[KMG]. */
> +       switch (*p) {
> +       case '@':
> +               *start = memparse(p + 1, &p);
> +               return 0;
> +       default:
> +               /*
> +                * If w/o offset, only size specified, immovable_mem=nn[KMG]
> +                * has the same behaviour as immovable_mem=nn[KMG]@0. It means
> +                * the region starts from 0.
> +                */
> +               *start = 0;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static void mem_avoid_memmap(char *str)
>  {
>         static int i;
> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>                 memmap_too_large = true;
>  }
>
> -static int handle_mem_memmap(void)
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +static void parse_immovable_mem_regions(char *str)
> +{
> +       static int i;
> +
> +       while (str && (i < MAX_IMMOVABLE_MEM)) {
> +               int rc;
> +               unsigned long long start, size;
> +               char *k = strchr(str, ',');
> +
> +               if (k)
> +                       *k++ = 0;
> +
> +               rc = parse_immovable_mem(str, &start, &size);
> +               if (rc < 0)
> +                       break;
> +               str = k;
> +
> +               immovable_mem[i].start = start;
> +               immovable_mem[i].size = size;
> +               i++;
> +       }
> +       num_immovable_region = i;
> +}
> +#else
> +static inline void parse_immovable_mem_regions(char *str)
> +{
> +}
> +#endif
> +
> +static int handle_mem_filter(void)
>  {
>         char *args = (char *)get_cmd_line_ptr();
>         size_t len = strlen((char *)args);
> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>         char *param, *val;
>         u64 mem_size;
>
> -       if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> +       if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> +           !strstr(args, "immovable_mem="))
>                 return 0;
>
>         tmp_cmdline = malloc(len + 1);
> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>
>                 if (!strcmp(param, "memmap")) {
>                         mem_avoid_memmap(val);
> +               } else if (!strcmp(param, "immovable_mem")) {
> +                       parse_immovable_mem_regions(val);
>                 } else if (!strcmp(param, "mem")) {
>                         char *p = val;
>
> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>         /* We don't need to set a mapping for setup_data. */
>
>         /* Mark the memmap regions we need to avoid */
> -       handle_mem_memmap();
> +       handle_mem_filter();
>
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>         /* Make sure video RAM can be used. */
> --
> 2.14.3
>
>
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Chao Fan]

On Tue, Dec 05, 2017 at 11:42:42AM -0800, Kees Cook wrote:
>On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>> In current code, kaslr may choose the memory region in movable
>> nodes to extract kernel, which will make the nodes can't be hot-removed.
>> To solve it, we can specify the memory region in immovable node.
>> Create immovable_mem to store the regions in immovable_mem, where should
>> be chosen by kaslr.
>>
>> Multiple regions can be specified, comma delimited.
>> Considering the usage of memory, only support for 4 regions.
>> 4 regions contains 2 nodes at least, enough for kernel to extract.
>>
>> Also change the "handle_mem_memmap" to "handle_mem_filter", since
>> it will not only handle memmap parameter now.
>

Hi Kees,

Thanks for your reply.

>I would put all immovable region functions and variables behind
>#ifdefs. e.g. parse_immovable_mem() is unused without

Yes, you are right.

>CONFIG_MEMORY_HOTPLUG. I also wonder if it might be possible to reuse
>some of the parsing code and just pass in which array you're updating

I will try to change it in the next version.

Thanks,
Chao Fan

>(i.e. memmap array vs immovable array).
>



>-Kees
>
>>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 77 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index a63fbc25ce84..0bbbaf5f6370 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -108,6 +108,15 @@ enum mem_avoid_index {
>>
>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>
>> +/* Only supporting at most 4 immovable memory regions with kaslr */
>> +#define MAX_IMMOVABLE_MEM      4
>> +
>> +/* Store the memory regions in immovable node */
>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> +
>> +/* The immovable regions user specify, not more than 4 */
>> +static int num_immovable_region;
>> +
>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>  {
>>         /* Item one is entirely before item two. */
>> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>>         return -EINVAL;
>>  }
>>
>> +static int parse_immovable_mem(char *p,
>> +                              unsigned long long *start,
>> +                              unsigned long long *size)
>> +{
>> +       char *oldp;
>> +
>> +       if (!p)
>> +               return -EINVAL;
>> +
>> +       oldp = p;
>> +       *size = memparse(p, &p);
>> +       if (p == oldp)
>> +               return -EINVAL;
>> +
>> +       /* We support nn[KMG]@ss[KMG] and nn[KMG]. */
>> +       switch (*p) {
>> +       case '@':
>> +               *start = memparse(p + 1, &p);
>> +               return 0;
>> +       default:
>> +               /*
>> +                * If w/o offset, only size specified, immovable_mem=nn[KMG]
>> +                * has the same behaviour as immovable_mem=nn[KMG]@0. It means
>> +                * the region starts from 0.
>> +                */
>> +               *start = 0;
>> +               return 0;
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>> +
>>  static void mem_avoid_memmap(char *str)
>>  {
>>         static int i;
>> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>>                 memmap_too_large = true;
>>  }
>>
>> -static int handle_mem_memmap(void)
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +static void parse_immovable_mem_regions(char *str)
>> +{
>> +       static int i;
>> +
>> +       while (str && (i < MAX_IMMOVABLE_MEM)) {
>> +               int rc;
>> +               unsigned long long start, size;
>> +               char *k = strchr(str, ',');
>> +
>> +               if (k)
>> +                       *k++ = 0;
>> +
>> +               rc = parse_immovable_mem(str, &start, &size);
>> +               if (rc < 0)
>> +                       break;
>> +               str = k;
>> +
>> +               immovable_mem[i].start = start;
>> +               immovable_mem[i].size = size;
>> +               i++;
>> +       }
>> +       num_immovable_region = i;
>> +}
>> +#else
>> +static inline void parse_immovable_mem_regions(char *str)
>> +{
>> +}
>> +#endif
>> +
>> +static int handle_mem_filter(void)
>>  {
>>         char *args = (char *)get_cmd_line_ptr();
>>         size_t len = strlen((char *)args);
>> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>>         char *param, *val;
>>         u64 mem_size;
>>
>> -       if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>> +       if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>> +           !strstr(args, "immovable_mem="))
>>                 return 0;
>>
>>         tmp_cmdline = malloc(len + 1);
>> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>>
>>                 if (!strcmp(param, "memmap")) {
>>                         mem_avoid_memmap(val);
>> +               } else if (!strcmp(param, "immovable_mem")) {
>> +                       parse_immovable_mem_regions(val);
>>                 } else if (!strcmp(param, "mem")) {
>>                         char *p = val;
>>
>> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>         /* We don't need to set a mapping for setup_data. */
>>
>>         /* Mark the memmap regions we need to avoid */
>> -       handle_mem_memmap();
>> +       handle_mem_filter();
>>
>>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>>         /* Make sure video RAM can be used. */
>> --
>> 2.14.3
>>
>>
>>
>
>
>
>-- 
>Kees Cook
>Pixel Security
>
>

Re: [PATCH v3 4/4] document: change the document for immovable_mem

[Chao Fan]

On Tue, Dec 05, 2017 at 09:36:24AM -0800, Randy Dunlap wrote:
>On 12/05/2017 12:52 AM, Chao Fan wrote:
>> Add the document for the change of new parameter
>> immovable_mem=nn[KMG]@ss[KMG].
>> 
>> Cc: linux-doc@vger.kernel.org
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index b74e13312fdc..eea755af697f 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2355,6 +2355,15 @@
>>  			allocations which rules out almost all kernel
>>  			allocations. Use with caution!
>>  
>> +	immovable_mem=nn[KMG]@ss[KMG]
>
>	immovable_mem=nn[KMG][@ss[KMG]]
>
>> +			[KNL] Force usage of a specific region of memory.
>> +			Make memory hotplug  work well with KASLR.
>
>			                   ^^only one space, please
>
>> +			Region of memory in immovable node is from ss to ss+nn.> +			Multiple regions can be specified, comma delimited.
>			If ss is omitted, it defaults to 0.

Sorry for the mistake. Will change it in next version.

Thanks,
Chao Fan

>> +			Notice: we support 4 regions at most now.
>> +			Example:
>> +			immovable_mem=1G,500M@2G,1G@4G
>> +
>>  	MTD_Partition=	[MTD]
>>  			Format: <name>,<region-number>,<size>,<offset>
>>  
>> 
>
>
>-- 
>~Randy
>
>

Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Baoquan He]

On 12/05/17 at 11:40am, Kees Cook wrote:
> On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> > If there is no immovable memory region specified, go on the old code.
> > There are several conditons:
> > 1. CONFIG_MEMORY_HOTPLUG is not specified to y.
> > 2. immovable_mem= is not specified.
> >
> > Otherwise, calculate the intersecting between memmap entry and
> > immovable memory.
> 
> Instead of copy/pasting code between process_efi_entries() and
> process_e820_entries(), I'd rather that process_mem_region() is
> modified to deal with immovable regions.

If put it into process_mem_region(), one level of loop is added. How
about changing it like below. If no immovable_mem, just process the
region in process_immovable_mem(). This we don't need to touch
process_mem_region().


>From 9ae3f5ab0e2f129757495af2412bd52dcf86aa46 Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Wed, 6 Dec 2017 17:24:55 +0800
Subject: [PATCH] KASLR: change code

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/kaslr.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 13d26b859c69..73b1562a7439 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -638,13 +638,23 @@ static bool process_immovable_mem(struct mem_vector region,
 				  unsigned long long minimum,
 				  unsigned long long image_size)
 {
-	int i;
+	/* If no immovable_mem stored, use region directly */
+	if (num_immovable_region == 0) {
+		process_mem_region(&entry, minimum, image_size);
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+
+		return 0;
+	}
 
 	/*
 	 * Walk all immovable regions, and filter the intersection
 	 * to process_mem_region.
 	 */
-	for (i = 0; i < num_immovable_region; i++) {
+	for (int i = 0; i < num_immovable_region; i++) {
 		struct mem_vector entry;
 		unsigned long long start, end, entry_end;
 
@@ -738,14 +748,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 		region.start = md->phys_addr;
 		region.size = md->num_pages << EFI_PAGE_SHIFT;
 
-		/* If no immovable_mem stored, use region directly */
-		if (num_immovable_region == 0) {
-			process_mem_region(&region, minimum, image_size);
-			if (slot_area_index == MAX_SLOT_AREA) {
-				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
-				break;
-			}
-		} else if (process_immovable_mem(region, minimum, image_size))
+		if (process_immovable_mem(region, minimum, image_size))
 			break;
 	}
 	return true;
@@ -774,14 +777,7 @@ static void process_e820_entries(unsigned long minimum,
 		region.start = entry->addr;
 		region.size = entry->size;
 
-		/* If no immovable_mem stored, use region directly */
-		if (num_immovable_region == 0) {
-			process_mem_region(&region, minimum, image_size);
-			if (slot_area_index == MAX_SLOT_AREA) {
-				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
-				break;
-			}
-		} else if (process_immovable_mem(region, minimum, image_size))
+		if (process_immovable_mem(region, minimum, image_size))
 			break;
 	}
 }
-- 
2.5.5

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Baoquan He]

Hi Chao,

Yes, now the code looks much better than the last version.

On 12/05/17 at 04:51pm, Chao Fan wrote:
> In current code, kaslr may choose the memory region in movable
> nodes to extract kernel, which will make the nodes can't be hot-removed.
> To solve it, we can specify the memory region in immovable node.
> Create immovable_mem to store the regions in immovable_mem, where should
> be chosen by kaslr.
> 
> Multiple regions can be specified, comma delimited.
> Considering the usage of memory, only support for 4 regions.
> 4 regions contains 2 nodes at least, enough for kernel to extract.
> 
> Also change the "handle_mem_memmap" to "handle_mem_filter", since
> it will not only handle memmap parameter now.

One concern is whether it will fail to do KASLR if only specify
"movable_node". Surely in this case it won't fail system, just hotplug
memory might be impacted if kernel is located on that, will FJ mind
this? And what if only specify 'immovable_mem=' but without 'movable_node'?

Thanks
Baoquan

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index a63fbc25ce84..0bbbaf5f6370 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -108,6 +108,15 @@ enum mem_avoid_index {
>  
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>  
> +/* Only supporting at most 4 immovable memory regions with kaslr */
> +#define MAX_IMMOVABLE_MEM	4
> +
> +/* Store the memory regions in immovable node */
> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> +
> +/* The immovable regions user specify, not more than 4 */
> +static int num_immovable_region;
> +
>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>  {
>  	/* Item one is entirely before item two. */
> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>  	return -EINVAL;
>  }
>  
> +static int parse_immovable_mem(char *p,
> +			       unsigned long long *start,
> +			       unsigned long long *size)
> +{
> +	char *oldp;
> +
> +	if (!p)
> +		return -EINVAL;
> +
> +	oldp = p;
> +	*size = memparse(p, &p);
> +	if (p == oldp)
> +		return -EINVAL;
> +
> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
> +	switch (*p) {
> +	case '@':
> +		*start = memparse(p + 1, &p);
> +		return 0;
> +	default:
> +		/*
> +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
> +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
> +		 * the region starts from 0.
> +		 */
> +		*start = 0;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static void mem_avoid_memmap(char *str)
>  {
>  	static int i;
> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>  		memmap_too_large = true;
>  }
>  
> -static int handle_mem_memmap(void)
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +static void parse_immovable_mem_regions(char *str)
> +{
> +	static int i;
> +
> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
> +		int rc;
> +		unsigned long long start, size;
> +		char *k = strchr(str, ',');
> +
> +		if (k)
> +			*k++ = 0;
> +
> +		rc = parse_immovable_mem(str, &start, &size);
> +		if (rc < 0)
> +			break;
> +		str = k;
> +
> +		immovable_mem[i].start = start;
> +		immovable_mem[i].size = size;
> +		i++;
> +	}
> +	num_immovable_region = i;
> +}
> +#else
> +static inline void parse_immovable_mem_regions(char *str)
> +{
> +}
> +#endif
> +
> +static int handle_mem_filter(void)
>  {
>  	char *args = (char *)get_cmd_line_ptr();
>  	size_t len = strlen((char *)args);
> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>  	char *param, *val;
>  	u64 mem_size;
>  
> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> +	    !strstr(args, "immovable_mem="))
>  		return 0;
>  
>  	tmp_cmdline = malloc(len + 1);
> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>  
>  		if (!strcmp(param, "memmap")) {
>  			mem_avoid_memmap(val);
> +		} else if (!strcmp(param, "immovable_mem")) {
> +			parse_immovable_mem_regions(val);
>  		} else if (!strcmp(param, "mem")) {
>  			char *p = val;
>  
> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	/* We don't need to set a mapping for setup_data. */
>  
>  	/* Mark the memmap regions we need to avoid */
> -	handle_mem_memmap();
> +	handle_mem_filter();
>  
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>  	/* Make sure video RAM can be used. */
> -- 
> 2.14.3
> 
> 
> 

Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Chao Fan]

On Wed, Dec 06, 2017 at 05:28:00PM +0800, Baoquan He wrote:
>On 12/05/17 at 11:40am, Kees Cook wrote:
>> On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>> > If there is no immovable memory region specified, go on the old code.
>> > There are several conditons:
>> > 1. CONFIG_MEMORY_HOTPLUG is not specified to y.
>> > 2. immovable_mem= is not specified.
>> >
>> > Otherwise, calculate the intersecting between memmap entry and
>> > immovable memory.
>> 
>> Instead of copy/pasting code between process_efi_entries() and
>> process_e820_entries(), I'd rather that process_mem_region() is
>> modified to deal with immovable regions.
>
>If put it into process_mem_region(), one level of loop is added. How

Yes, one new loop will add ahead of the while() in process_mem_region
then the code may look like:

@@ -509,6 +555,24 @@ static void process_mem_region(struct mem_vector *entry,
        region.start = cur_entry.start;
        region.size = cur_entry.size;

+#ifdef CONFIG_MEMORY_HOTPLUG
+next:
+       if (num_immovable_mem > 0) {
+               unsigned long long start, reg_end;
+
+               if (!mem_overlaps(&entry, &immovable_mem[i]))
+                       goto out;
+
+               start = immovable_mem[i].start;
+               end = start + immovable_mem[i].size;
+
+               region.start = clamp(cur_entry.start, start, end);
+               reg_end = clamp(cur_entry.start + cur_entry.size, start, end);
+
+               region.size = region_end - region.start;
+       }
+#endif
+
        /* Give up if slot area array is full. */
        while (slot_area_index < MAX_SLOT_AREA) {
                start_orig = region.start;
@@ -522,7 +586,7 @@ static void process_mem_region(struct mem_vector *entry,

                /* Did we raise the address above the passed in memory entry? */
                if (region.start > cur_entry.start + cur_entry.size)
-                       return;
+                       goto out;

                /* Reduce size by any delta from the original address. */
                region.size -= region.start - start_orig;
@@ -534,12 +598,12 @@ static void process_mem_region(struct mem_vector *entry,

                /* Return if region can't contain decompressed kernel */
                if (region.size < image_size)
-                       return;
+                       goto out;

                /* If nothing overlaps, store the region and return. */
                if (!mem_avoid_overlap(&region, &overlap)) {
                        store_slot_info(&region, image_size);
-                       return;
+                       goto out;
                }

                /* Store beginning of region if holds at least image_size. */
		@@ -553,12 +617,20 @@ static void process_mem_region(struct mem_vector *entry,

                /* Return if overlap extends to or past end of region. */
                if (overlap.start + overlap.size >= region.start + region.size)
-                       return;
+                       goto out;

                /* Clip off the overlapping region and start over. */
                region.size -= overlap.start - region.start + overlap.size;
                region.start = overlap.start + overlap.size;
        }
+
+out:
+#ifdef CONFIG_MEMORY_HOTPLUG
+       i++;
+       if (i < num_immovable_mem)
+               goto next;
+#endif
+       return;
 }

>about changing it like below. If no immovable_mem, just process the
>region in process_immovable_mem(). This we don't need to touch
>process_mem_region().

Yes, Baoquan's method will make all change be in one function.
Kees, how do you think, which is better?

Thanks,
Chao Fan

>
>
>From 9ae3f5ab0e2f129757495af2412bd52dcf86aa46 Mon Sep 17 00:00:00 2001
>From: Baoquan He <bhe@redhat.com>
>Date: Wed, 6 Dec 2017 17:24:55 +0800
>Subject: [PATCH] KASLR: change code
>
>Signed-off-by: Baoquan He <bhe@redhat.com>
>---
> arch/x86/boot/compressed/kaslr.c | 32 ++++++++++++++------------------
> 1 file changed, 14 insertions(+), 18 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 13d26b859c69..73b1562a7439 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -638,13 +638,23 @@ static bool process_immovable_mem(struct mem_vector region,
> 				  unsigned long long minimum,
> 				  unsigned long long image_size)
> {
>-	int i;
>+	/* If no immovable_mem stored, use region directly */
>+	if (num_immovable_region == 0) {
>+		process_mem_region(&entry, minimum, image_size);
>+
>+		if (slot_area_index == MAX_SLOT_AREA) {
>+			debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>+			return 1;
>+		}
>+
>+		return 0;
>+	}
> 
> 	/*
> 	 * Walk all immovable regions, and filter the intersection
> 	 * to process_mem_region.
> 	 */
>-	for (i = 0; i < num_immovable_region; i++) {
>+	for (int i = 0; i < num_immovable_region; i++) {
> 		struct mem_vector entry;
> 		unsigned long long start, end, entry_end;
> 
>@@ -738,14 +748,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
> 		region.start = md->phys_addr;
> 		region.size = md->num_pages << EFI_PAGE_SHIFT;
> 
>-		/* If no immovable_mem stored, use region directly */
>-		if (num_immovable_region == 0) {
>-			process_mem_region(&region, minimum, image_size);
>-			if (slot_area_index == MAX_SLOT_AREA) {
>-				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>-				break;
>-			}
>-		} else if (process_immovable_mem(region, minimum, image_size))
>+		if (process_immovable_mem(region, minimum, image_size))
> 			break;
> 	}
> 	return true;
>@@ -774,14 +777,7 @@ static void process_e820_entries(unsigned long minimum,
> 		region.start = entry->addr;
> 		region.size = entry->size;
> 
>-		/* If no immovable_mem stored, use region directly */
>-		if (num_immovable_region == 0) {
>-			process_mem_region(&region, minimum, image_size);
>-			if (slot_area_index == MAX_SLOT_AREA) {
>-				debug_putstr("Aborted memmap scan (slot_areas full)!\n");
>-				break;
>-			}
>-		} else if (process_immovable_mem(region, minimum, image_size))
>+		if (process_immovable_mem(region, minimum, image_size))
> 			break;
> 	}
> }
>-- 
>2.5.5
>
>
>

Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Kees Cook]

On Wed, Dec 6, 2017 at 2:02 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> On Wed, Dec 06, 2017 at 05:28:00PM +0800, Baoquan He wrote:
>>On 12/05/17 at 11:40am, Kees Cook wrote:
>>> On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>>> > If there is no immovable memory region specified, go on the old code.
>>> > There are several conditons:
>>> > 1. CONFIG_MEMORY_HOTPLUG is not specified to y.
>>> > 2. immovable_mem= is not specified.
>>> >
>>> > Otherwise, calculate the intersecting between memmap entry and
>>> > immovable memory.
>>>
>>> Instead of copy/pasting code between process_efi_entries() and
>>> process_e820_entries(), I'd rather that process_mem_region() is
>>> modified to deal with immovable regions.
>>
>>If put it into process_mem_region(), one level of loop is added. How
>
> Yes, one new loop will add ahead of the while() in process_mem_region
> then the code may look like:
>
> @@ -509,6 +555,24 @@ static void process_mem_region(struct mem_vector *entry,
>         region.start = cur_entry.start;
>         region.size = cur_entry.size;
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +next:
> +       if (num_immovable_mem > 0) {
> +               unsigned long long start, reg_end;
> +
> +               if (!mem_overlaps(&entry, &immovable_mem[i]))
> +                       goto out;
> +
> +               start = immovable_mem[i].start;
> +               end = start + immovable_mem[i].size;
> +
> +               region.start = clamp(cur_entry.start, start, end);
> +               reg_end = clamp(cur_entry.start + cur_entry.size, start, end);
> +
> +               region.size = region_end - region.start;
> +       }
> +#endif
> +
>         /* Give up if slot area array is full. */
>         while (slot_area_index < MAX_SLOT_AREA) {
>                 start_orig = region.start;
> @@ -522,7 +586,7 @@ static void process_mem_region(struct mem_vector *entry,
>
>                 /* Did we raise the address above the passed in memory entry? */
>                 if (region.start > cur_entry.start + cur_entry.size)
> -                       return;
> +                       goto out;
>
>                 /* Reduce size by any delta from the original address. */
>                 region.size -= region.start - start_orig;
> @@ -534,12 +598,12 @@ static void process_mem_region(struct mem_vector *entry,
>
>                 /* Return if region can't contain decompressed kernel */
>                 if (region.size < image_size)
> -                       return;
> +                       goto out;
>
>                 /* If nothing overlaps, store the region and return. */
>                 if (!mem_avoid_overlap(&region, &overlap)) {
>                         store_slot_info(&region, image_size);
> -                       return;
> +                       goto out;
>                 }
>
>                 /* Store beginning of region if holds at least image_size. */
>                 @@ -553,12 +617,20 @@ static void process_mem_region(struct mem_vector *entry,
>
>                 /* Return if overlap extends to or past end of region. */
>                 if (overlap.start + overlap.size >= region.start + region.size)
> -                       return;
> +                       goto out;
>
>                 /* Clip off the overlapping region and start over. */
>                 region.size -= overlap.start - region.start + overlap.size;
>                 region.start = overlap.start + overlap.size;
>         }
> +
> +out:
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +       i++;
> +       if (i < num_immovable_mem)
> +               goto next;
> +#endif
> +       return;
>  }
>
>>about changing it like below. If no immovable_mem, just process the
>>region in process_immovable_mem(). This we don't need to touch
>>process_mem_region().
>
> Yes, Baoquan's method will make all change be in one function.
> Kees, how do you think, which is better?

I prefer Baoquan's approach, though I don't like the function names.
:) Perhaps rename process_mem_region() to slots_count() (to match
slots_fetch_random()) and rename process_immovable_mem() to
process_mem_region().

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3 2/4] kaslr: calculate the memory region in immovable node

[Chao Fan]

On Wed, Dec 06, 2017 at 04:11:04PM -0800, Kees Cook wrote:
>On Wed, Dec 6, 2017 at 2:02 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>> On Wed, Dec 06, 2017 at 05:28:00PM +0800, Baoquan He wrote:
>>>On 12/05/17 at 11:40am, Kees Cook wrote:
>>>> On Tue, Dec 5, 2017 at 12:51 AM, Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>>>> > If there is no immovable memory region specified, go on the old code.
>>>> > There are several conditons:
>>>> > 1. CONFIG_MEMORY_HOTPLUG is not specified to y.
>>>> > 2. immovable_mem= is not specified.
>>>> >
>>>> > Otherwise, calculate the intersecting between memmap entry and
>>>> > immovable memory.
>>>>
>>>> Instead of copy/pasting code between process_efi_entries() and
>>>> process_e820_entries(), I'd rather that process_mem_region() is
>>>> modified to deal with immovable regions.
>>>
>>>If put it into process_mem_region(), one level of loop is added. How
>>
>> Yes, one new loop will add ahead of the while() in process_mem_region
>> then the code may look like:
>>
>> @@ -509,6 +555,24 @@ static void process_mem_region(struct mem_vector *entry,
>>         region.start = cur_entry.start;
>>         region.size = cur_entry.size;
>>
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +next:
>> +       if (num_immovable_mem > 0) {
>> +               unsigned long long start, reg_end;
>> +
>> +               if (!mem_overlaps(&entry, &immovable_mem[i]))
>> +                       goto out;
>> +
>> +               start = immovable_mem[i].start;
>> +               end = start + immovable_mem[i].size;
>> +
>> +               region.start = clamp(cur_entry.start, start, end);
>> +               reg_end = clamp(cur_entry.start + cur_entry.size, start, end);
>> +
>> +               region.size = region_end - region.start;
>> +       }
>> +#endif
>> +
>>         /* Give up if slot area array is full. */
>>         while (slot_area_index < MAX_SLOT_AREA) {
>>                 start_orig = region.start;
>> @@ -522,7 +586,7 @@ static void process_mem_region(struct mem_vector *entry,
>>
>>                 /* Did we raise the address above the passed in memory entry? */
>>                 if (region.start > cur_entry.start + cur_entry.size)
>> -                       return;
>> +                       goto out;
>>
>>                 /* Reduce size by any delta from the original address. */
>>                 region.size -= region.start - start_orig;
>> @@ -534,12 +598,12 @@ static void process_mem_region(struct mem_vector *entry,
>>
>>                 /* Return if region can't contain decompressed kernel */
>>                 if (region.size < image_size)
>> -                       return;
>> +                       goto out;
>>
>>                 /* If nothing overlaps, store the region and return. */
>>                 if (!mem_avoid_overlap(&region, &overlap)) {
>>                         store_slot_info(&region, image_size);
>> -                       return;
>> +                       goto out;
>>                 }
>>
>>                 /* Store beginning of region if holds at least image_size. */
>>                 @@ -553,12 +617,20 @@ static void process_mem_region(struct mem_vector *entry,
>>
>>                 /* Return if overlap extends to or past end of region. */
>>                 if (overlap.start + overlap.size >= region.start + region.size)
>> -                       return;
>> +                       goto out;
>>
>>                 /* Clip off the overlapping region and start over. */
>>                 region.size -= overlap.start - region.start + overlap.size;
>>                 region.start = overlap.start + overlap.size;
>>         }
>> +
>> +out:
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +       i++;
>> +       if (i < num_immovable_mem)
>> +               goto next;
>> +#endif
>> +       return;
>>  }
>>
>>>about changing it like below. If no immovable_mem, just process the
>>>region in process_immovable_mem(). This we don't need to touch
>>>process_mem_region().
>>
>> Yes, Baoquan's method will make all change be in one function.
>> Kees, how do you think, which is better?
>
>I prefer Baoquan's approach, though I don't like the function names.
>:) Perhaps rename process_mem_region() to slots_count() (to match
>slots_fetch_random()) and rename process_immovable_mem() to
>process_mem_region().

Thanks for your review, I will change and send the new version.

Thanks,
Chao Fan

>
>-Kees
>
>-- 
>Kees Cook
>Pixel Security
>
>

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Chao Fan]

On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
>Hi Chao,
>
>Yes, now the code looks much better than the last version.
>
>On 12/05/17 at 04:51pm, Chao Fan wrote:
>> In current code, kaslr may choose the memory region in movable
>> nodes to extract kernel, which will make the nodes can't be hot-removed.
>> To solve it, we can specify the memory region in immovable node.
>> Create immovable_mem to store the regions in immovable_mem, where should
>> be chosen by kaslr.
>> 
>> Multiple regions can be specified, comma delimited.
>> Considering the usage of memory, only support for 4 regions.
>> 4 regions contains 2 nodes at least, enough for kernel to extract.
>> 
>> Also change the "handle_mem_memmap" to "handle_mem_filter", since
>> it will not only handle memmap parameter now.
>
>One concern is whether it will fail to do KASLR if only specify

Sorry, I think I have not understood your point.
So if there is something wrong, please let me know.

I don't think if only specify "movable_node" will fail KASLR.
Since in this patchset(3/4), only disable kernel mirror. KASLR in
current upstream code didn't parse "movable_node".

>"movable_node". Surely in this case it won't fail system, just hotplug
>memory might be impacted if kernel is located on that, will FJ mind

Yes, it's the reason why I make this patchset.
In my personal understanding, "movable_node" is a beginning why I make
this patchset, but not the whole reason.
Only "movable_node" specified might cause hotplug memory can't be
removed if kernel is located on that, so we need the help of
"immovable_mem=". "movable_node" help hotplug memory can be removed, and
"immovable_mem=" works for the same target, but just in kaslr.
So up to now, there is not a very tight coupling between "movable_node"
and "immovable_mem=". The independence of "immovable_mem=" is that,
help kaslr selects the right regions, avoid the memory in hotpluggable
NUMA nodes, which causes the memory can't removed. It's a independent
reason why we need a parameter like "immovable_mem=".
So I think we should also handle it if only specify "immovable_mem="
without "movable_node".

Thanks,
Chao Fan

>this? And what if only specify 'immovable_mem=' but without 'movable_node'?
>
>Thanks
>Baoquan
>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 77 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index a63fbc25ce84..0bbbaf5f6370 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -108,6 +108,15 @@ enum mem_avoid_index {
>>  
>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>  
>> +/* Only supporting at most 4 immovable memory regions with kaslr */
>> +#define MAX_IMMOVABLE_MEM	4
>> +
>> +/* Store the memory regions in immovable node */
>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> +
>> +/* The immovable regions user specify, not more than 4 */
>> +static int num_immovable_region;
>> +
>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>  {
>>  	/* Item one is entirely before item two. */
>> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>>  	return -EINVAL;
>>  }
>>  
>> +static int parse_immovable_mem(char *p,
>> +			       unsigned long long *start,
>> +			       unsigned long long *size)
>> +{
>> +	char *oldp;
>> +
>> +	if (!p)
>> +		return -EINVAL;
>> +
>> +	oldp = p;
>> +	*size = memparse(p, &p);
>> +	if (p == oldp)
>> +		return -EINVAL;
>> +
>> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
>> +	switch (*p) {
>> +	case '@':
>> +		*start = memparse(p + 1, &p);
>> +		return 0;
>> +	default:
>> +		/*
>> +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
>> +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
>> +		 * the region starts from 0.
>> +		 */
>> +		*start = 0;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static void mem_avoid_memmap(char *str)
>>  {
>>  	static int i;
>> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>>  		memmap_too_large = true;
>>  }
>>  
>> -static int handle_mem_memmap(void)
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +static void parse_immovable_mem_regions(char *str)
>> +{
>> +	static int i;
>> +
>> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>> +		int rc;
>> +		unsigned long long start, size;
>> +		char *k = strchr(str, ',');
>> +
>> +		if (k)
>> +			*k++ = 0;
>> +
>> +		rc = parse_immovable_mem(str, &start, &size);
>> +		if (rc < 0)
>> +			break;
>> +		str = k;
>> +
>> +		immovable_mem[i].start = start;
>> +		immovable_mem[i].size = size;
>> +		i++;
>> +	}
>> +	num_immovable_region = i;
>> +}
>> +#else
>> +static inline void parse_immovable_mem_regions(char *str)
>> +{
>> +}
>> +#endif
>> +
>> +static int handle_mem_filter(void)
>>  {
>>  	char *args = (char *)get_cmd_line_ptr();
>>  	size_t len = strlen((char *)args);
>> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>>  	char *param, *val;
>>  	u64 mem_size;
>>  
>> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>> +	    !strstr(args, "immovable_mem="))
>>  		return 0;
>>  
>>  	tmp_cmdline = malloc(len + 1);
>> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>>  
>>  		if (!strcmp(param, "memmap")) {
>>  			mem_avoid_memmap(val);
>> +		} else if (!strcmp(param, "immovable_mem")) {
>> +			parse_immovable_mem_regions(val);
>>  		} else if (!strcmp(param, "mem")) {
>>  			char *p = val;
>>  
>> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>  	/* We don't need to set a mapping for setup_data. */
>>  
>>  	/* Mark the memmap regions we need to avoid */
>> -	handle_mem_memmap();
>> +	handle_mem_filter();
>>  
>>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>>  	/* Make sure video RAM can be used. */
>> -- 
>> 2.14.3
>> 
>> 
>> 
>
>

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Baoquan He]

On 12/07/17 at 10:53am, Chao Fan wrote:
> On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
> >Hi Chao,
> >
> >Yes, now the code looks much better than the last version.
> >
> >On 12/05/17 at 04:51pm, Chao Fan wrote:
> >> In current code, kaslr may choose the memory region in movable
> >> nodes to extract kernel, which will make the nodes can't be hot-removed.
> >> To solve it, we can specify the memory region in immovable node.
> >> Create immovable_mem to store the regions in immovable_mem, where should
> >> be chosen by kaslr.
> >> 
> >> Multiple regions can be specified, comma delimited.
> >> Considering the usage of memory, only support for 4 regions.
> >> 4 regions contains 2 nodes at least, enough for kernel to extract.
> >> 
> >> Also change the "handle_mem_memmap" to "handle_mem_filter", since
> >> it will not only handle memmap parameter now.
> >
> >One concern is whether it will fail to do KASLR if only specify
> 
> Sorry, I think I have not understood your point.
> So if there is something wrong, please let me know.

What I meant is whether we need check 'movable_node' and 
'immovable_mem=' being specified together. If only specify 'movable_node',
we may need to return and do not do kaslr or do not do physical kaslr
since kernel could be located on movable mem region.

Otherwise it will do physical kaslr anyway, memory hotplug will be
impacted later.

> 
> I don't think if only specify "movable_node" will fail KASLR.
> Since in this patchset(3/4), only disable kernel mirror. KASLR in
> current upstream code didn't parse "movable_node".
> 
> >"movable_node". Surely in this case it won't fail system, just hotplug
> >memory might be impacted if kernel is located on that, will FJ mind
> 
> Yes, it's the reason why I make this patchset.
> In my personal understanding, "movable_node" is a beginning why I make
> this patchset, but not the whole reason.
> Only "movable_node" specified might cause hotplug memory can't be
> removed if kernel is located on that, so we need the help of
> "immovable_mem=". "movable_node" help hotplug memory can be removed, and
> "immovable_mem=" works for the same target, but just in kaslr.
> So up to now, there is not a very tight coupling between "movable_node"
> and "immovable_mem=". The independence of "immovable_mem=" is that,
> help kaslr selects the right regions, avoid the memory in hotpluggable
> NUMA nodes, which causes the memory can't removed. It's a independent
> reason why we need a parameter like "immovable_mem=".
> So I think we should also handle it if only specify "immovable_mem="
> without "movable_node".
> 
> Thanks,
> Chao Fan
> 
> >this? And what if only specify 'immovable_mem=' but without 'movable_node'?
> >
> >Thanks
> >Baoquan
> >
> >> 
> >> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> >> ---
> >>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 77 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> >> index a63fbc25ce84..0bbbaf5f6370 100644
> >> --- a/arch/x86/boot/compressed/kaslr.c
> >> +++ b/arch/x86/boot/compressed/kaslr.c
> >> @@ -108,6 +108,15 @@ enum mem_avoid_index {
> >>  
> >>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
> >>  
> >> +/* Only supporting at most 4 immovable memory regions with kaslr */
> >> +#define MAX_IMMOVABLE_MEM	4
> >> +
> >> +/* Store the memory regions in immovable node */
> >> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> >> +
> >> +/* The immovable regions user specify, not more than 4 */
> >> +static int num_immovable_region;
> >> +
> >>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> >>  {
> >>  	/* Item one is entirely before item two. */
> >> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >>  	return -EINVAL;
> >>  }
> >>  
> >> +static int parse_immovable_mem(char *p,
> >> +			       unsigned long long *start,
> >> +			       unsigned long long *size)
> >> +{
> >> +	char *oldp;
> >> +
> >> +	if (!p)
> >> +		return -EINVAL;
> >> +
> >> +	oldp = p;
> >> +	*size = memparse(p, &p);
> >> +	if (p == oldp)
> >> +		return -EINVAL;
> >> +
> >> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
> >> +	switch (*p) {
> >> +	case '@':
> >> +		*start = memparse(p + 1, &p);
> >> +		return 0;
> >> +	default:
> >> +		/*
> >> +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
> >> +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
> >> +		 * the region starts from 0.
> >> +		 */
> >> +		*start = 0;
> >> +		return 0;
> >> +	}
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +
> >>  static void mem_avoid_memmap(char *str)
> >>  {
> >>  	static int i;
> >> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
> >>  		memmap_too_large = true;
> >>  }
> >>  
> >> -static int handle_mem_memmap(void)
> >> +#ifdef CONFIG_MEMORY_HOTPLUG
> >> +static void parse_immovable_mem_regions(char *str)
> >> +{
> >> +	static int i;
> >> +
> >> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
> >> +		int rc;
> >> +		unsigned long long start, size;
> >> +		char *k = strchr(str, ',');
> >> +
> >> +		if (k)
> >> +			*k++ = 0;
> >> +
> >> +		rc = parse_immovable_mem(str, &start, &size);
> >> +		if (rc < 0)
> >> +			break;
> >> +		str = k;
> >> +
> >> +		immovable_mem[i].start = start;
> >> +		immovable_mem[i].size = size;
> >> +		i++;
> >> +	}
> >> +	num_immovable_region = i;
> >> +}
> >> +#else
> >> +static inline void parse_immovable_mem_regions(char *str)
> >> +{
> >> +}
> >> +#endif
> >> +
> >> +static int handle_mem_filter(void)
> >>  {
> >>  	char *args = (char *)get_cmd_line_ptr();
> >>  	size_t len = strlen((char *)args);
> >> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
> >>  	char *param, *val;
> >>  	u64 mem_size;
> >>  
> >> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> >> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> >> +	    !strstr(args, "immovable_mem="))
> >>  		return 0;
> >>  
> >>  	tmp_cmdline = malloc(len + 1);
> >> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
> >>  
> >>  		if (!strcmp(param, "memmap")) {
> >>  			mem_avoid_memmap(val);
> >> +		} else if (!strcmp(param, "immovable_mem")) {
> >> +			parse_immovable_mem_regions(val);
> >>  		} else if (!strcmp(param, "mem")) {
> >>  			char *p = val;
> >>  
> >> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >>  	/* We don't need to set a mapping for setup_data. */
> >>  
> >>  	/* Mark the memmap regions we need to avoid */
> >> -	handle_mem_memmap();
> >> +	handle_mem_filter();
> >>  
> >>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
> >>  	/* Make sure video RAM can be used. */
> >> -- 
> >> 2.14.3
> >> 
> >> 
> >> 
> >
> >
> 
> 

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Chao Fan]

On Thu, Dec 07, 2017 at 11:09:24AM +0800, Baoquan He wrote:
>On 12/07/17 at 10:53am, Chao Fan wrote:
>> On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
>> >Hi Chao,
>> >
>> >Yes, now the code looks much better than the last version.
>> >
>> >On 12/05/17 at 04:51pm, Chao Fan wrote:
>> >> In current code, kaslr may choose the memory region in movable
>> >> nodes to extract kernel, which will make the nodes can't be hot-removed.
>> >> To solve it, we can specify the memory region in immovable node.
>> >> Create immovable_mem to store the regions in immovable_mem, where should
>> >> be chosen by kaslr.
>> >> 
>> >> Multiple regions can be specified, comma delimited.
>> >> Considering the usage of memory, only support for 4 regions.
>> >> 4 regions contains 2 nodes at least, enough for kernel to extract.
>> >> 
>> >> Also change the "handle_mem_memmap" to "handle_mem_filter", since
>> >> it will not only handle memmap parameter now.
>> >
>> >One concern is whether it will fail to do KASLR if only specify
>> 
>> Sorry, I think I have not understood your point.
>> So if there is something wrong, please let me know.
>
>What I meant is whether we need check 'movable_node' and 
>'immovable_mem=' being specified together. If only specify 'movable_node',
>we may need to return and do not do kaslr or do not do physical kaslr
>since kernel could be located on movable mem region.

I think both are OK and have reasons, and I tend to not return.
Because if there is a parameter can solve the problem, but not specified.
It's a problem of user-level.
How do you think?

Thanks,
Chao Fan

>
>Otherwise it will do physical kaslr anyway, memory hotplug will be
>impacted later.
>
>> 
>> I don't think if only specify "movable_node" will fail KASLR.
>> Since in this patchset(3/4), only disable kernel mirror. KASLR in
>> current upstream code didn't parse "movable_node".
>> 
>> >"movable_node". Surely in this case it won't fail system, just hotplug
>> >memory might be impacted if kernel is located on that, will FJ mind
>> 
>> Yes, it's the reason why I make this patchset.
>> In my personal understanding, "movable_node" is a beginning why I make
>> this patchset, but not the whole reason.
>> Only "movable_node" specified might cause hotplug memory can't be
>> removed if kernel is located on that, so we need the help of
>> "immovable_mem=". "movable_node" help hotplug memory can be removed, and
>> "immovable_mem=" works for the same target, but just in kaslr.
>> So up to now, there is not a very tight coupling between "movable_node"
>> and "immovable_mem=". The independence of "immovable_mem=" is that,
>> help kaslr selects the right regions, avoid the memory in hotpluggable
>> NUMA nodes, which causes the memory can't removed. It's a independent
>> reason why we need a parameter like "immovable_mem=".
>> So I think we should also handle it if only specify "immovable_mem="
>> without "movable_node".
>> 
>> Thanks,
>> Chao Fan
>> 
>> >this? And what if only specify 'immovable_mem=' but without 'movable_node'?
>> >
>> >Thanks
>> >Baoquan
>> >
>> >> 
>> >> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> >> ---
>> >>  arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>> >>  1 file changed, 77 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> >> index a63fbc25ce84..0bbbaf5f6370 100644
>> >> --- a/arch/x86/boot/compressed/kaslr.c
>> >> +++ b/arch/x86/boot/compressed/kaslr.c
>> >> @@ -108,6 +108,15 @@ enum mem_avoid_index {
>> >>  
>> >>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>> >>  
>> >> +/* Only supporting at most 4 immovable memory regions with kaslr */
>> >> +#define MAX_IMMOVABLE_MEM	4
>> >> +
>> >> +/* Store the memory regions in immovable node */
>> >> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> >> +
>> >> +/* The immovable regions user specify, not more than 4 */
>> >> +static int num_immovable_region;
>> >> +
>> >>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>> >>  {
>> >>  	/* Item one is entirely before item two. */
>> >> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>> >>  	return -EINVAL;
>> >>  }
>> >>  
>> >> +static int parse_immovable_mem(char *p,
>> >> +			       unsigned long long *start,
>> >> +			       unsigned long long *size)
>> >> +{
>> >> +	char *oldp;
>> >> +
>> >> +	if (!p)
>> >> +		return -EINVAL;
>> >> +
>> >> +	oldp = p;
>> >> +	*size = memparse(p, &p);
>> >> +	if (p == oldp)
>> >> +		return -EINVAL;
>> >> +
>> >> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
>> >> +	switch (*p) {
>> >> +	case '@':
>> >> +		*start = memparse(p + 1, &p);
>> >> +		return 0;
>> >> +	default:
>> >> +		/*
>> >> +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
>> >> +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
>> >> +		 * the region starts from 0.
>> >> +		 */
>> >> +		*start = 0;
>> >> +		return 0;
>> >> +	}
>> >> +
>> >> +	return -EINVAL;
>> >> +}
>> >> +
>> >>  static void mem_avoid_memmap(char *str)
>> >>  {
>> >>  	static int i;
>> >> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>> >>  		memmap_too_large = true;
>> >>  }
>> >>  
>> >> -static int handle_mem_memmap(void)
>> >> +#ifdef CONFIG_MEMORY_HOTPLUG
>> >> +static void parse_immovable_mem_regions(char *str)
>> >> +{
>> >> +	static int i;
>> >> +
>> >> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>> >> +		int rc;
>> >> +		unsigned long long start, size;
>> >> +		char *k = strchr(str, ',');
>> >> +
>> >> +		if (k)
>> >> +			*k++ = 0;
>> >> +
>> >> +		rc = parse_immovable_mem(str, &start, &size);
>> >> +		if (rc < 0)
>> >> +			break;
>> >> +		str = k;
>> >> +
>> >> +		immovable_mem[i].start = start;
>> >> +		immovable_mem[i].size = size;
>> >> +		i++;
>> >> +	}
>> >> +	num_immovable_region = i;
>> >> +}
>> >> +#else
>> >> +static inline void parse_immovable_mem_regions(char *str)
>> >> +{
>> >> +}
>> >> +#endif
>> >> +
>> >> +static int handle_mem_filter(void)
>> >>  {
>> >>  	char *args = (char *)get_cmd_line_ptr();
>> >>  	size_t len = strlen((char *)args);
>> >> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>> >>  	char *param, *val;
>> >>  	u64 mem_size;
>> >>  
>> >> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>> >> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>> >> +	    !strstr(args, "immovable_mem="))
>> >>  		return 0;
>> >>  
>> >>  	tmp_cmdline = malloc(len + 1);
>> >> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>> >>  
>> >>  		if (!strcmp(param, "memmap")) {
>> >>  			mem_avoid_memmap(val);
>> >> +		} else if (!strcmp(param, "immovable_mem")) {
>> >> +			parse_immovable_mem_regions(val);
>> >>  		} else if (!strcmp(param, "mem")) {
>> >>  			char *p = val;
>> >>  
>> >> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>> >>  	/* We don't need to set a mapping for setup_data. */
>> >>  
>> >>  	/* Mark the memmap regions we need to avoid */
>> >> -	handle_mem_memmap();
>> >> +	handle_mem_filter();
>> >>  
>> >>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>> >>  	/* Make sure video RAM can be used. */
>> >> -- 
>> >> 2.14.3
>> >> 
>> >> 
>> >> 
>> >
>> >
>> 
>> 
>
>

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Dou Liyang]

Hi All,

At 12/07/2017 11:56 AM, Chao Fan wrote:
> On Thu, Dec 07, 2017 at 11:09:24AM +0800, Baoquan He wrote:
>> On 12/07/17 at 10:53am, Chao Fan wrote:
>>> On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
>>>> Hi Chao,
>>>>
>>>> Yes, now the code looks much better than the last version.
>>>>
>>>> On 12/05/17 at 04:51pm, Chao Fan wrote:
>>>>> In current code, kaslr may choose the memory region in movable
>>>>> nodes to extract kernel, which will make the nodes can't be hot-removed.
>>>>> To solve it, we can specify the memory region in immovable node.
>>>>> Create immovable_mem to store the regions in immovable_mem, where should
>>>>> be chosen by kaslr.
>>>>>
>>>>> Multiple regions can be specified, comma delimited.
>>>>> Considering the usage of memory, only support for 4 regions.
>>>>> 4 regions contains 2 nodes at least, enough for kernel to extract.
>>>>>
>>>>> Also change the "handle_mem_memmap" to "handle_mem_filter", since
>>>>> it will not only handle memmap parameter now.
>>>>
>>>> One concern is whether it will fail to do KASLR if only specify
>>>
>>> Sorry, I think I have not understood your point.
>>> So if there is something wrong, please let me know.
>>
>> What I meant is whether we need check 'movable_node' and
>> 'immovable_mem=' being specified together. If only specify 'movable_node',
>> we may need to return and do not do kaslr or do not do physical kaslr
>> since kernel could be located on movable mem region.
>
Indeed.

If *immovable_mem* is valid only when Kernel supports both
KASLR and Node hotplug(movable_node). we need check them together:

...
   else if (!strcmp(param, "movable_node")) {
	if (!strcmp(param, "immovable_mem"))
		parse_immovable_mem_regions(val);
	else
		//no KASLR or no node hotplug?

}
...

> I think both are OK and have reasons, and I tend to not return.
> Because if there is a parameter can solve the problem, but not specified.
> It's a problem of user-level.
> How do you think?
> 

Seems we should clarify the scope of 'immovable_mem=' and document it.

Thanks,
	dou

> Thanks,
> Chao Fan
> 
>>
>> Otherwise it will do physical kaslr anyway, memory hotplug will be
>> impacted later.
>>
>>>
>>> I don't think if only specify "movable_node" will fail KASLR.
>>> Since in this patchset(3/4), only disable kernel mirror. KASLR in
>>> current upstream code didn't parse "movable_node".
>>>
>>>> "movable_node". Surely in this case it won't fail system, just hotplug
>>>> memory might be impacted if kernel is located on that, will FJ mind
>>>
>>> Yes, it's the reason why I make this patchset.
>>> In my personal understanding, "movable_node" is a beginning why I make
>>> this patchset, but not the whole reason.
>>> Only "movable_node" specified might cause hotplug memory can't be
>>> removed if kernel is located on that, so we need the help of
>>> "immovable_mem=". "movable_node" help hotplug memory can be removed, and
>>> "immovable_mem=" works for the same target, but just in kaslr.
>>> So up to now, there is not a very tight coupling between "movable_node"
>>> and "immovable_mem=". The independence of "immovable_mem=" is that,
>>> help kaslr selects the right regions, avoid the memory in hotpluggable
>>> NUMA nodes, which causes the memory can't removed. It's a independent
>>> reason why we need a parameter like "immovable_mem=".
>>> So I think we should also handle it if only specify "immovable_mem="
>>> without "movable_node".
>>>
>>> Thanks,
>>> Chao Fan
>>>
>>>> this? And what if only specify 'immovable_mem=' but without 'movable_node'?
>>>>
>>>> Thanks
>>>> Baoquan
>>>>
>>>>>
>>>>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>>>>> ---
>>>>>   arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>>>>>   1 file changed, 77 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>>>>> index a63fbc25ce84..0bbbaf5f6370 100644
>>>>> --- a/arch/x86/boot/compressed/kaslr.c
>>>>> +++ b/arch/x86/boot/compressed/kaslr.c
>>>>> @@ -108,6 +108,15 @@ enum mem_avoid_index {
>>>>>   
>>>>>   static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>>>>   
>>>>> +/* Only supporting at most 4 immovable memory regions with kaslr */
>>>>> +#define MAX_IMMOVABLE_MEM	4
>>>>> +
>>>>> +/* Store the memory regions in immovable node */
>>>>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>>>>> +
>>>>> +/* The immovable regions user specify, not more than 4 */
>>>>> +static int num_immovable_region;
>>>>> +
>>>>>   static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>>>>   {
>>>>>   	/* Item one is entirely before item two. */
>>>>> @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>>>>>   	return -EINVAL;
>>>>>   }
>>>>>   
>>>>> +static int parse_immovable_mem(char *p,
>>>>> +			       unsigned long long *start,
>>>>> +			       unsigned long long *size)
>>>>> +{
>>>>> +	char *oldp;
>>>>> +
>>>>> +	if (!p)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	oldp = p;
>>>>> +	*size = memparse(p, &p);
>>>>> +	if (p == oldp)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
>>>>> +	switch (*p) {
>>>>> +	case '@':
>>>>> +		*start = memparse(p + 1, &p);
>>>>> +		return 0;
>>>>> +	default:
>>>>> +		/*
>>>>> +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
>>>>> +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
>>>>> +		 * the region starts from 0.
>>>>> +		 */
>>>>> +		*start = 0;
>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>> +	return -EINVAL;
>>>>> +}
>>>>> +
>>>>>   static void mem_avoid_memmap(char *str)
>>>>>   {
>>>>>   	static int i;
>>>>> @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>>>>>   		memmap_too_large = true;
>>>>>   }
>>>>>   
>>>>> -static int handle_mem_memmap(void)
>>>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>>>> +static void parse_immovable_mem_regions(char *str)
>>>>> +{
>>>>> +	static int i;
>>>>> +
>>>>> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>>>>> +		int rc;
>>>>> +		unsigned long long start, size;
>>>>> +		char *k = strchr(str, ',');
>>>>> +
>>>>> +		if (k)
>>>>> +			*k++ = 0;
>>>>> +
>>>>> +		rc = parse_immovable_mem(str, &start, &size);
>>>>> +		if (rc < 0)
>>>>> +			break;
>>>>> +		str = k;
>>>>> +
>>>>> +		immovable_mem[i].start = start;
>>>>> +		immovable_mem[i].size = size;
>>>>> +		i++;
>>>>> +	}
>>>>> +	num_immovable_region = i;
>>>>> +}
>>>>> +#else
>>>>> +static inline void parse_immovable_mem_regions(char *str)
>>>>> +{
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +static int handle_mem_filter(void)
>>>>>   {
>>>>>   	char *args = (char *)get_cmd_line_ptr();
>>>>>   	size_t len = strlen((char *)args);
>>>>> @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>>>>>   	char *param, *val;
>>>>>   	u64 mem_size;
>>>>>   
>>>>> -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>>>>> +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>>>>> +	    !strstr(args, "immovable_mem="))
>>>>>   		return 0;
>>>>>   
>>>>>   	tmp_cmdline = malloc(len + 1);
>>>>> @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>>>>>   
>>>>>   		if (!strcmp(param, "memmap")) {
>>>>>   			mem_avoid_memmap(val);
>>>>> +		} else if (!strcmp(param, "immovable_mem")) {
>>>>> +			parse_immovable_mem_regions(val);
>>>>>   		} else if (!strcmp(param, "mem")) {
>>>>>   			char *p = val;
>>>>>   
>>>>> @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>>>>   	/* We don't need to set a mapping for setup_data. */
>>>>>   
>>>>>   	/* Mark the memmap regions we need to avoid */
>>>>> -	handle_mem_memmap();
>>>>> +	handle_mem_filter();
>>>>>   
>>>>>   #ifdef CONFIG_X86_VERBOSE_BOOTUP
>>>>>   	/* Make sure video RAM can be used. */
>>>>> -- 
>>>>> 2.14.3
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Baoquan He]

On 12/07/17 at 12:16pm, Dou Liyang wrote:
> Hi All,
> 
> At 12/07/2017 11:56 AM, Chao Fan wrote:
> > On Thu, Dec 07, 2017 at 11:09:24AM +0800, Baoquan He wrote:
> > > On 12/07/17 at 10:53am, Chao Fan wrote:
> > > > On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
> > > > > Hi Chao,
> > > > > 
> > > > > Yes, now the code looks much better than the last version.
> > > > > 
> > > > > On 12/05/17 at 04:51pm, Chao Fan wrote:
> > > > > > In current code, kaslr may choose the memory region in movable
> > > > > > nodes to extract kernel, which will make the nodes can't be hot-removed.
> > > > > > To solve it, we can specify the memory region in immovable node.
> > > > > > Create immovable_mem to store the regions in immovable_mem, where should
> > > > > > be chosen by kaslr.
> > > > > > 
> > > > > > Multiple regions can be specified, comma delimited.
> > > > > > Considering the usage of memory, only support for 4 regions.
> > > > > > 4 regions contains 2 nodes at least, enough for kernel to extract.
> > > > > > 
> > > > > > Also change the "handle_mem_memmap" to "handle_mem_filter", since
> > > > > > it will not only handle memmap parameter now.
> > > > > 
> > > > > One concern is whether it will fail to do KASLR if only specify
> > > > 
> > > > Sorry, I think I have not understood your point.
> > > > So if there is something wrong, please let me know.
> > > 
> > > What I meant is whether we need check 'movable_node' and
> > > 'immovable_mem=' being specified together. If only specify 'movable_node',
> > > we may need to return and do not do kaslr or do not do physical kaslr
> > > since kernel could be located on movable mem region.
> > 
> Indeed.
> 
> If *immovable_mem* is valid only when Kernel supports both
> KASLR and Node hotplug(movable_node). we need check them together:
> 
> ...
>   else if (!strcmp(param, "movable_node")) {
> 	if (!strcmp(param, "immovable_mem"))
> 		parse_immovable_mem_regions(val);
> 	else
> 		//no KASLR or no node hotplug?
> 
> }

Yes, I meant this. We can skip kernel physical address randomization,
the virtual address can still be randomized.

> ...
> 
> > I think both are OK and have reasons, and I tend to not return.
> > Because if there is a parameter can solve the problem, but not specified.
> > It's a problem of user-level.
> > How do you think?
> > 
> 
> Seems we should clarify the scope of 'immovable_mem=' and document it.
> 
> Thanks,
> 	dou
> 
> > Thanks,
> > Chao Fan
> > 
> > > 
> > > Otherwise it will do physical kaslr anyway, memory hotplug will be
> > > impacted later.
> > > 
> > > > 
> > > > I don't think if only specify "movable_node" will fail KASLR.
> > > > Since in this patchset(3/4), only disable kernel mirror. KASLR in
> > > > current upstream code didn't parse "movable_node".
> > > > 
> > > > > "movable_node". Surely in this case it won't fail system, just hotplug
> > > > > memory might be impacted if kernel is located on that, will FJ mind
> > > > 
> > > > Yes, it's the reason why I make this patchset.
> > > > In my personal understanding, "movable_node" is a beginning why I make
> > > > this patchset, but not the whole reason.
> > > > Only "movable_node" specified might cause hotplug memory can't be
> > > > removed if kernel is located on that, so we need the help of
> > > > "immovable_mem=". "movable_node" help hotplug memory can be removed, and
> > > > "immovable_mem=" works for the same target, but just in kaslr.
> > > > So up to now, there is not a very tight coupling between "movable_node"
> > > > and "immovable_mem=". The independence of "immovable_mem=" is that,
> > > > help kaslr selects the right regions, avoid the memory in hotpluggable
> > > > NUMA nodes, which causes the memory can't removed. It's a independent
> > > > reason why we need a parameter like "immovable_mem=".
> > > > So I think we should also handle it if only specify "immovable_mem="
> > > > without "movable_node".
> > > > 
> > > > Thanks,
> > > > Chao Fan
> > > > 
> > > > > this? And what if only specify 'immovable_mem=' but without 'movable_node'?
> > > > > 
> > > > > Thanks
> > > > > Baoquan
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> > > > > > ---
> > > > > >   arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
> > > > > >   1 file changed, 77 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > > > > > index a63fbc25ce84..0bbbaf5f6370 100644
> > > > > > --- a/arch/x86/boot/compressed/kaslr.c
> > > > > > +++ b/arch/x86/boot/compressed/kaslr.c
> > > > > > @@ -108,6 +108,15 @@ enum mem_avoid_index {
> > > > > >   static struct mem_vector mem_avoid[MEM_AVOID_MAX];
> > > > > > +/* Only supporting at most 4 immovable memory regions with kaslr */
> > > > > > +#define MAX_IMMOVABLE_MEM	4
> > > > > > +
> > > > > > +/* Store the memory regions in immovable node */
> > > > > > +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
> > > > > > +
> > > > > > +/* The immovable regions user specify, not more than 4 */
> > > > > > +static int num_immovable_region;
> > > > > > +
> > > > > >   static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> > > > > >   {
> > > > > >   	/* Item one is entirely before item two. */
> > > > > > @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > > > > >   	return -EINVAL;
> > > > > >   }
> > > > > > +static int parse_immovable_mem(char *p,
> > > > > > +			       unsigned long long *start,
> > > > > > +			       unsigned long long *size)
> > > > > > +{
> > > > > > +	char *oldp;
> > > > > > +
> > > > > > +	if (!p)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	oldp = p;
> > > > > > +	*size = memparse(p, &p);
> > > > > > +	if (p == oldp)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
> > > > > > +	switch (*p) {
> > > > > > +	case '@':
> > > > > > +		*start = memparse(p + 1, &p);
> > > > > > +		return 0;
> > > > > > +	default:
> > > > > > +		/*
> > > > > > +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
> > > > > > +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
> > > > > > +		 * the region starts from 0.
> > > > > > +		 */
> > > > > > +		*start = 0;
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	return -EINVAL;
> > > > > > +}
> > > > > > +
> > > > > >   static void mem_avoid_memmap(char *str)
> > > > > >   {
> > > > > >   	static int i;
> > > > > > @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
> > > > > >   		memmap_too_large = true;
> > > > > >   }
> > > > > > -static int handle_mem_memmap(void)
> > > > > > +#ifdef CONFIG_MEMORY_HOTPLUG
> > > > > > +static void parse_immovable_mem_regions(char *str)
> > > > > > +{
> > > > > > +	static int i;
> > > > > > +
> > > > > > +	while (str && (i < MAX_IMMOVABLE_MEM)) {
> > > > > > +		int rc;
> > > > > > +		unsigned long long start, size;
> > > > > > +		char *k = strchr(str, ',');
> > > > > > +
> > > > > > +		if (k)
> > > > > > +			*k++ = 0;
> > > > > > +
> > > > > > +		rc = parse_immovable_mem(str, &start, &size);
> > > > > > +		if (rc < 0)
> > > > > > +			break;
> > > > > > +		str = k;
> > > > > > +
> > > > > > +		immovable_mem[i].start = start;
> > > > > > +		immovable_mem[i].size = size;
> > > > > > +		i++;
> > > > > > +	}
> > > > > > +	num_immovable_region = i;
> > > > > > +}
> > > > > > +#else
> > > > > > +static inline void parse_immovable_mem_regions(char *str)
> > > > > > +{
> > > > > > +}
> > > > > > +#endif
> > > > > > +
> > > > > > +static int handle_mem_filter(void)
> > > > > >   {
> > > > > >   	char *args = (char *)get_cmd_line_ptr();
> > > > > >   	size_t len = strlen((char *)args);
> > > > > > @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
> > > > > >   	char *param, *val;
> > > > > >   	u64 mem_size;
> > > > > > -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> > > > > > +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
> > > > > > +	    !strstr(args, "immovable_mem="))
> > > > > >   		return 0;
> > > > > >   	tmp_cmdline = malloc(len + 1);
> > > > > > @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
> > > > > >   		if (!strcmp(param, "memmap")) {
> > > > > >   			mem_avoid_memmap(val);
> > > > > > +		} else if (!strcmp(param, "immovable_mem")) {
> > > > > > +			parse_immovable_mem_regions(val);
> > > > > >   		} else if (!strcmp(param, "mem")) {
> > > > > >   			char *p = val;
> > > > > > @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> > > > > >   	/* We don't need to set a mapping for setup_data. */
> > > > > >   	/* Mark the memmap regions we need to avoid */
> > > > > > -	handle_mem_memmap();
> > > > > > +	handle_mem_filter();
> > > > > >   #ifdef CONFIG_X86_VERBOSE_BOOTUP
> > > > > >   	/* Make sure video RAM can be used. */
> > > > > > -- 
> > > > > > 2.14.3
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> 
> 

Re: [PATCH v3 1/4] kaslr: add immovable_mem=nn[KMG]@ss[KMG] to specify extracting memory

[Chao Fan]

On Thu, Dec 07, 2017 at 12:58:06PM +0800, Baoquan He wrote:
>On 12/07/17 at 12:16pm, Dou Liyang wrote:
>> Hi All,
>> 
>> At 12/07/2017 11:56 AM, Chao Fan wrote:
>> > On Thu, Dec 07, 2017 at 11:09:24AM +0800, Baoquan He wrote:
>> > > On 12/07/17 at 10:53am, Chao Fan wrote:
>> > > > On Wed, Dec 06, 2017 at 05:35:57PM +0800, Baoquan He wrote:
>> > > > > Hi Chao,
>> > > > > 
>> > > > > Yes, now the code looks much better than the last version.
>> > > > > 
>> > > > > On 12/05/17 at 04:51pm, Chao Fan wrote:
>> > > > > > In current code, kaslr may choose the memory region in movable
>> > > > > > nodes to extract kernel, which will make the nodes can't be hot-removed.
>> > > > > > To solve it, we can specify the memory region in immovable node.
>> > > > > > Create immovable_mem to store the regions in immovable_mem, where should
>> > > > > > be chosen by kaslr.
>> > > > > > 
>> > > > > > Multiple regions can be specified, comma delimited.
>> > > > > > Considering the usage of memory, only support for 4 regions.
>> > > > > > 4 regions contains 2 nodes at least, enough for kernel to extract.
>> > > > > > 
>> > > > > > Also change the "handle_mem_memmap" to "handle_mem_filter", since
>> > > > > > it will not only handle memmap parameter now.
>> > > > > 
>> > > > > One concern is whether it will fail to do KASLR if only specify
>> > > > 
>> > > > Sorry, I think I have not understood your point.
>> > > > So if there is something wrong, please let me know.
>> > > 
>> > > What I meant is whether we need check 'movable_node' and
>> > > 'immovable_mem=' being specified together. If only specify 'movable_node',
>> > > we may need to return and do not do kaslr or do not do physical kaslr
>> > > since kernel could be located on movable mem region.
>> > 
>> Indeed.
>> 
>> If *immovable_mem* is valid only when Kernel supports both
>> KASLR and Node hotplug(movable_node). we need check them together:
>> 
>> ...
>>   else if (!strcmp(param, "movable_node")) {
>> 	if (!strcmp(param, "immovable_mem"))
>> 		parse_immovable_mem_regions(val);
>> 	else
>> 		//no KASLR or no node hotplug?
>> 
>> }
>
>Yes, I meant this. We can skip kernel physical address randomization,
>the virtual address can still be randomized.
>

Thanks Baoquan and Dou, I will make the new version.

Thanks,
Chao Fan

>> ...
>> 
>> > I think both are OK and have reasons, and I tend to not return.
>> > Because if there is a parameter can solve the problem, but not specified.
>> > It's a problem of user-level.
>> > How do you think?
>> > 
>> 
>> Seems we should clarify the scope of 'immovable_mem=' and document it.
>> 
>> Thanks,
>> 	dou
>> 
>> > Thanks,
>> > Chao Fan
>> > 
>> > > 
>> > > Otherwise it will do physical kaslr anyway, memory hotplug will be
>> > > impacted later.
>> > > 
>> > > > 
>> > > > I don't think if only specify "movable_node" will fail KASLR.
>> > > > Since in this patchset(3/4), only disable kernel mirror. KASLR in
>> > > > current upstream code didn't parse "movable_node".
>> > > > 
>> > > > > "movable_node". Surely in this case it won't fail system, just hotplug
>> > > > > memory might be impacted if kernel is located on that, will FJ mind
>> > > > 
>> > > > Yes, it's the reason why I make this patchset.
>> > > > In my personal understanding, "movable_node" is a beginning why I make
>> > > > this patchset, but not the whole reason.
>> > > > Only "movable_node" specified might cause hotplug memory can't be
>> > > > removed if kernel is located on that, so we need the help of
>> > > > "immovable_mem=". "movable_node" help hotplug memory can be removed, and
>> > > > "immovable_mem=" works for the same target, but just in kaslr.
>> > > > So up to now, there is not a very tight coupling between "movable_node"
>> > > > and "immovable_mem=". The independence of "immovable_mem=" is that,
>> > > > help kaslr selects the right regions, avoid the memory in hotpluggable
>> > > > NUMA nodes, which causes the memory can't removed. It's a independent
>> > > > reason why we need a parameter like "immovable_mem=".
>> > > > So I think we should also handle it if only specify "immovable_mem="
>> > > > without "movable_node".
>> > > > 
>> > > > Thanks,
>> > > > Chao Fan
>> > > > 
>> > > > > this? And what if only specify 'immovable_mem=' but without 'movable_node'?
>> > > > > 
>> > > > > Thanks
>> > > > > Baoquan
>> > > > > 
>> > > > > > 
>> > > > > > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> > > > > > ---
>> > > > > >   arch/x86/boot/compressed/kaslr.c | 80 ++++++++++++++++++++++++++++++++++++++--
>> > > > > >   1 file changed, 77 insertions(+), 3 deletions(-)
>> > > > > > 
>> > > > > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> > > > > > index a63fbc25ce84..0bbbaf5f6370 100644
>> > > > > > --- a/arch/x86/boot/compressed/kaslr.c
>> > > > > > +++ b/arch/x86/boot/compressed/kaslr.c
>> > > > > > @@ -108,6 +108,15 @@ enum mem_avoid_index {
>> > > > > >   static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>> > > > > > +/* Only supporting at most 4 immovable memory regions with kaslr */
>> > > > > > +#define MAX_IMMOVABLE_MEM	4
>> > > > > > +
>> > > > > > +/* Store the memory regions in immovable node */
>> > > > > > +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> > > > > > +
>> > > > > > +/* The immovable regions user specify, not more than 4 */
>> > > > > > +static int num_immovable_region;
>> > > > > > +
>> > > > > >   static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>> > > > > >   {
>> > > > > >   	/* Item one is entirely before item two. */
>> > > > > > @@ -168,6 +177,38 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>> > > > > >   	return -EINVAL;
>> > > > > >   }
>> > > > > > +static int parse_immovable_mem(char *p,
>> > > > > > +			       unsigned long long *start,
>> > > > > > +			       unsigned long long *size)
>> > > > > > +{
>> > > > > > +	char *oldp;
>> > > > > > +
>> > > > > > +	if (!p)
>> > > > > > +		return -EINVAL;
>> > > > > > +
>> > > > > > +	oldp = p;
>> > > > > > +	*size = memparse(p, &p);
>> > > > > > +	if (p == oldp)
>> > > > > > +		return -EINVAL;
>> > > > > > +
>> > > > > > +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
>> > > > > > +	switch (*p) {
>> > > > > > +	case '@':
>> > > > > > +		*start = memparse(p + 1, &p);
>> > > > > > +		return 0;
>> > > > > > +	default:
>> > > > > > +		/*
>> > > > > > +		 * If w/o offset, only size specified, immovable_mem=nn[KMG]
>> > > > > > +		 * has the same behaviour as immovable_mem=nn[KMG]@0. It means
>> > > > > > +		 * the region starts from 0.
>> > > > > > +		 */
>> > > > > > +		*start = 0;
>> > > > > > +		return 0;
>> > > > > > +	}
>> > > > > > +
>> > > > > > +	return -EINVAL;
>> > > > > > +}
>> > > > > > +
>> > > > > >   static void mem_avoid_memmap(char *str)
>> > > > > >   {
>> > > > > >   	static int i;
>> > > > > > @@ -207,7 +248,37 @@ static void mem_avoid_memmap(char *str)
>> > > > > >   		memmap_too_large = true;
>> > > > > >   }
>> > > > > > -static int handle_mem_memmap(void)
>> > > > > > +#ifdef CONFIG_MEMORY_HOTPLUG
>> > > > > > +static void parse_immovable_mem_regions(char *str)
>> > > > > > +{
>> > > > > > +	static int i;
>> > > > > > +
>> > > > > > +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>> > > > > > +		int rc;
>> > > > > > +		unsigned long long start, size;
>> > > > > > +		char *k = strchr(str, ',');
>> > > > > > +
>> > > > > > +		if (k)
>> > > > > > +			*k++ = 0;
>> > > > > > +
>> > > > > > +		rc = parse_immovable_mem(str, &start, &size);
>> > > > > > +		if (rc < 0)
>> > > > > > +			break;
>> > > > > > +		str = k;
>> > > > > > +
>> > > > > > +		immovable_mem[i].start = start;
>> > > > > > +		immovable_mem[i].size = size;
>> > > > > > +		i++;
>> > > > > > +	}
>> > > > > > +	num_immovable_region = i;
>> > > > > > +}
>> > > > > > +#else
>> > > > > > +static inline void parse_immovable_mem_regions(char *str)
>> > > > > > +{
>> > > > > > +}
>> > > > > > +#endif
>> > > > > > +
>> > > > > > +static int handle_mem_filter(void)
>> > > > > >   {
>> > > > > >   	char *args = (char *)get_cmd_line_ptr();
>> > > > > >   	size_t len = strlen((char *)args);
>> > > > > > @@ -215,7 +286,8 @@ static int handle_mem_memmap(void)
>> > > > > >   	char *param, *val;
>> > > > > >   	u64 mem_size;
>> > > > > > -	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>> > > > > > +	if (!strstr(args, "memmap=") && !strstr(args, "mem=") &&
>> > > > > > +	    !strstr(args, "immovable_mem="))
>> > > > > >   		return 0;
>> > > > > >   	tmp_cmdline = malloc(len + 1);
>> > > > > > @@ -240,6 +312,8 @@ static int handle_mem_memmap(void)
>> > > > > >   		if (!strcmp(param, "memmap")) {
>> > > > > >   			mem_avoid_memmap(val);
>> > > > > > +		} else if (!strcmp(param, "immovable_mem")) {
>> > > > > > +			parse_immovable_mem_regions(val);
>> > > > > >   		} else if (!strcmp(param, "mem")) {
>> > > > > >   			char *p = val;
>> > > > > > @@ -379,7 +453,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>> > > > > >   	/* We don't need to set a mapping for setup_data. */
>> > > > > >   	/* Mark the memmap regions we need to avoid */
>> > > > > > -	handle_mem_memmap();
>> > > > > > +	handle_mem_filter();
>> > > > > >   #ifdef CONFIG_X86_VERBOSE_BOOTUP
>> > > > > >   	/* Make sure video RAM can be used. */
>> > > > > > -- 
>> > > > > > 2.14.3
>> > > > > > 
>> > > > > > 
>> > > > > > 
>> > > > > 
>> > > > > 
>> > > > 
>> > > > 
>> > > 
>> > > 
>> > 
>> 
>> 
>
>