[PATCH v4 0/3] Handle memmap and mem kernel options in boot stage kaslr

[PATCH v4 0/3] Handle memmap and mem kernel options in boot stage kaslr

[Baoquan He]

People reported kernel panic occurs during system boots up with mem boot option.
After checking code, several problems are found about memmap= and mem= in boot stage
kaslr.

*) In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"), only one memmap
   entry is considered and only the last one if multiple memmap entries are specified.

*) mem= and memmap=nn[KMG] are not considered yet. They are used to limit max address
   of system. Kernel can't be randomized to be above the limit.

*) kernel-parameters.txt doesn't tell the updated behaviour of memmap=.

This patchset tries to solve above issues, and it sits on top of
tip:x86/boot branch.

Changelog

v3->v4:
 *Code improved patch 1/3 according to Kees's suggestion.

 *Add 'Fall through' in switch case of parse_memmap() which
  is suggestd by Kees.

v2->v3:
  No functionality change in this round.
 *Use local static variable insted of global variable
  mem_avoid_memmap_index in patch 1/3.

 *Fix a typo in patch 3/3.

v1->v2:
 *The original patch 1/4 has been put in tip:x86/boot and no update,
  so it's not included in this post.

 *Use patch log Ingo reorganized.

 *lib/ctype.c and lib/cmdline.c are needed for kaslr.c, while those
  EXPORT_SYMBOL(x) contained caused failure of build on 32-bit allmodconfig:
  ......
  ld: -r and -shared may not be used together
  scripts/Makefile.build:294: recipe for target 'arch/x86/boot/compressed/kaslr.o' failed
  ......
  Disabling the symbol exporting removes the build failure.

 *Use dynamic allocation to allocate memory to contain copied kernel cmdline
  buffer, it's implemented in include/linux/decompress/mm.h.

Baoquan He (3):
  KASLR: Parse all memmap entries in cmdline
  KASLR: Handle memory limit specified by memmap and mem option
  Documentation/kernel-parameters.txt: Update 'memmap=' option
    description

 Documentation/admin-guide/kernel-parameters.txt |   9 ++
 arch/x86/boot/compressed/cmdline.c              |   2 +-
 arch/x86/boot/compressed/kaslr.c                | 191 ++++++++++++++++--------
 arch/x86/boot/string.c                          |   8 +
 4 files changed, 144 insertions(+), 66 deletions(-)

-- 
2.5.5

[PATCH v4 1/3] KASLR: Parse all memmap entries in cmdline

[Baoquan He]

In commit:

  f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")

... the memmap= option is parsed so that KASLR can avoid those reserved
regions. It uses cmdline_find_option() to get the value if memmap=
is specified, however the problem is that cmdline_find_option() can only
find the last entry if multiple memmap entries are provided. This
is not correct.

In this patch, the whole cmdline will be scanned to search each
memmap, all of them will be parsed and handled.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/cmdline.c |   2 +-
 arch/x86/boot/compressed/kaslr.c   | 136 ++++++++++++++++++++++---------------
 arch/x86/boot/string.c             |   8 +++
 3 files changed, 91 insertions(+), 55 deletions(-)

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index 73ccf63..9dc1ce6 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -13,7 +13,7 @@ static inline char rdfs8(addr_t addr)
 	return *((char *)(fs + addr));
 }
 #include "../cmdline.c"
-static unsigned long get_cmd_line_ptr(void)
+unsigned long get_cmd_line_ptr(void)
 {
 	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
 
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 54c24f0..106e13b 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -9,16 +9,41 @@
  * contain the entire properly aligned running kernel image.
  *
  */
+
+/*
+ * isspace() in linux/ctype.h is expected by next_args() to filter
+ * out "space/lf/tab". While boot/ctype.h conflicts with linux/ctype.h,
+ * since isdigit() is implemented in both of them. Hence disable it
+ * here.
+ */
+#define BOOT_CTYPE_H
+
+/*
+ * _ctype[] in lib/ctype.c is needed by isspace() of linux/ctype.h.
+ * While both lib/ctype.c and lib/cmdline.c will bring EXPORT_SYMBOL
+ * which is meaningless and will cause compiling error in some cases.
+ * So do not include linux/export.h and define EXPORT_SYMBOL(sym)
+ * as empty.
+ */
+#define _LINUX_EXPORT_H
+#define EXPORT_SYMBOL(sym)
+
 #include "misc.h"
 #include "error.h"
-#include "../boot.h"
 
 #include <generated/compile.h>
 #include <linux/module.h>
 #include <linux/uts.h>
 #include <linux/utsname.h>
+#include <linux/ctype.h>
 #include <generated/utsrelease.h>
 
+/* Macros used by the included decompressor code below. */
+#define STATIC
+#include <linux/decompress/mm.h>
+
+extern unsigned long get_cmd_line_ptr(void);
+
 /* Simplified build-specific string for starting entropy. */
 static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
 		LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
@@ -62,6 +87,7 @@ struct mem_vector {
 
 static bool memmap_too_large;
 
+
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
 	MEM_AVOID_INITRD,
@@ -85,49 +111,14 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
 	return true;
 }
 
-/**
- *	_memparse - Parse a string with mem suffixes into a number
- *	@ptr: Where parse begins
- *	@retptr: (output) Optional pointer to next char after parse completes
- *
- *	Parses a string into a number.  The number stored at @ptr is
- *	potentially suffixed with K, M, G, T, P, E.
- */
-static unsigned long long _memparse(const char *ptr, char **retptr)
+char *skip_spaces(const char *str)
 {
-	char *endptr;	/* Local pointer to end of parsed string */
-
-	unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-	switch (*endptr) {
-	case 'E':
-	case 'e':
-		ret <<= 10;
-	case 'P':
-	case 'p':
-		ret <<= 10;
-	case 'T':
-	case 't':
-		ret <<= 10;
-	case 'G':
-	case 'g':
-		ret <<= 10;
-	case 'M':
-	case 'm':
-		ret <<= 10;
-	case 'K':
-	case 'k':
-		ret <<= 10;
-		endptr++;
-	default:
-		break;
-	}
-
-	if (retptr)
-		*retptr = endptr;
-
-	return ret;
+	while (isspace(*str))
+		++str;
+	return (char *)str;
 }
+#include "../../../../lib/ctype.c"
+#include "../../../../lib/cmdline.c"
 
 static int
 parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
@@ -142,7 +133,7 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 		return -EINVAL;
 
 	oldp = p;
-	*size = _memparse(p, &p);
+	*size = memparse(p, &p);
 	if (p == oldp)
 		return -EINVAL;
 
@@ -155,27 +146,21 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 	case '#':
 	case '$':
 	case '!':
-		*start = _memparse(p + 1, &p);
+		*start = memparse(p + 1, &p);
 		return 0;
 	}
 
 	return -EINVAL;
 }
 
-static void mem_avoid_memmap(void)
+static void mem_avoid_memmap(char *str)
 {
-	char arg[128];
+	static int i;
 	int rc;
-	int i;
-	char *str;
 
-	/* See if we have any memmap areas */
-	rc = cmdline_find_option("memmap", arg, sizeof(arg));
-	if (rc <= 0)
+	if (i >= MAX_MEMMAP_REGIONS)
 		return;
 
-	i = 0;
-	str = arg;
 	while (str && (i < MAX_MEMMAP_REGIONS)) {
 		int rc;
 		unsigned long long start, size;
@@ -202,6 +187,49 @@ static void mem_avoid_memmap(void)
 		memmap_too_large = true;
 }
 
+
+/*
+ * handle_mem_memmap will also cover 'mem=' issue in next patch. Will remove
+ * this note later.
+ */
+static int handle_mem_memmap(void)
+{
+	char *args = (char *)get_cmd_line_ptr();
+	size_t len = strlen((char *)args);
+	char *tmp_cmdline;
+	char *param, *val;
+
+	if (!strstr(args, "memmap="))
+		return 0;
+
+	tmp_cmdline = malloc(len + 1);
+	if (!tmp_cmdline )
+		error("Failed to allocate space for tmp_cmdline");
+
+	memcpy(tmp_cmdline, args, len);
+	tmp_cmdline[len] = 0;
+	args = tmp_cmdline;
+
+	/* Chew leading spaces */
+	args = skip_spaces(args);
+
+	while (*args) {
+		args = next_arg(args, &param, &val);
+		/* Stop at -- */
+		if (!val && strcmp(param, "--") == 0) {
+			warn("Only '--' specified in cmdline");
+			free(tmp_cmdline);
+			return -1;
+		}
+
+		if (!strcmp(param, "memmap"))
+			mem_avoid_memmap(val);
+	}
+
+	free(tmp_cmdline);
+	return 0;
+}
+
 /*
  * In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
  * The mem_avoid array is used to store the ranges that need to be avoided
@@ -323,7 +351,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 */
-	mem_avoid_memmap();
+	handle_mem_memmap();
 
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 5457b02..630e366 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -122,6 +122,14 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
 	return result;
 }
 
+long simple_strtol(const char *cp, char **endp, unsigned int base)
+{
+	if (*cp == '-')
+		return -simple_strtoull(cp + 1, endp, base);
+
+	return simple_strtoull(cp, endp, base);
+}
+
 /**
  * strlen - Find the length of a string
  * @s: The string to be sized
-- 
2.5.5

[PATCH v4 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Baoquan He]

Option mem= will limit the max address a system can use and any memory
region above the limit will be removed.

Furthermore, memmap=nn[KMG] which has no offset specified has the same
behaviour as mem=.

KASLR needs to consider this when choosing the random position for
decompressing the kernel.

This patch implements that.

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

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 106e13b..0637e85 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -88,6 +88,10 @@ struct mem_vector {
 static bool memmap_too_large;
 
 
+/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
+unsigned long long mem_limit = ULLONG_MAX;
+
+
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
 	MEM_AVOID_INITRD,
@@ -138,16 +142,24 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 		return -EINVAL;
 
 	switch (*p) {
-	case '@':
-		/* Skip this region, usable */
-		*start = 0;
-		*size = 0;
-		return 0;
 	case '#':
 	case '$':
 	case '!':
 		*start = memparse(p + 1, &p);
 		return 0;
+	case '@':	/* Fall through: */
+		/*
+		 * memmap=nn@ss specifies usable region, should be skipped
+		 */
+		*size = 0;
+	default:
+		/*
+		 * If w/o offset, only size specified, memmap=nn[KMG] has the
+		 * same behaviour as mem=nn[KMG]. It limits the max address
+		 * system can use. Region above the limit should be avoided.
+		 */
+		*start = 0;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -173,9 +185,14 @@ static void mem_avoid_memmap(char *str)
 		if (rc < 0)
 			break;
 		str = k;
-		/* A usable region that should not be skipped */
-		if (size == 0)
+
+		if (start == 0) {
+			/* Store the specified memory limit if size > 0 */
+			if (size > 0)
+				mem_limit = size;
+
 			continue;
+		}
 
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
@@ -187,19 +204,15 @@ static void mem_avoid_memmap(char *str)
 		memmap_too_large = true;
 }
 
-
-/*
- * handle_mem_memmap will also cover 'mem=' issue in next patch. Will remove
- * this note later.
- */
 static int handle_mem_memmap(void)
 {
 	char *args = (char *)get_cmd_line_ptr();
 	size_t len = strlen((char *)args);
 	char *tmp_cmdline;
 	char *param, *val;
+	u64 mem_size;
 
-	if (!strstr(args, "memmap="))
+	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
 		return 0;
 
 	tmp_cmdline = malloc(len + 1);
@@ -222,8 +235,20 @@ static int handle_mem_memmap(void)
 			return -1;
 		}
 
-		if (!strcmp(param, "memmap"))
+		if (!strcmp(param, "memmap")) {
 			mem_avoid_memmap(val);
+		} else if (!strcmp(param, "mem")) {
+			char *p = val;
+
+			if (!strcmp(p, "nopentium"))
+				continue;
+			mem_size = memparse(p, &p);
+			if (mem_size == 0) {
+				free(tmp_cmdline);
+				return -EINVAL;
+			}
+			mem_limit = mem_size;
+		}
 	}
 
 	free(tmp_cmdline);
@@ -460,7 +485,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
 {
 	struct mem_vector region, overlap;
 	struct slot_area slot_area;
-	unsigned long start_orig;
+	unsigned long start_orig, end;
+	struct boot_e820_entry cur_entry;
 
 	/* Skip non-RAM entries. */
 	if (entry->type != E820_TYPE_RAM)
@@ -474,8 +500,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
 	if (entry->addr + entry->size < minimum)
 		return;
 
-	region.start = entry->addr;
-	region.size = entry->size;
+	/* Ignore entries above memory limit */
+	end = min(entry->size + entry->addr, mem_limit);
+	if (entry->addr >= end)
+		return;
+	cur_entry.addr = entry->addr;
+	cur_entry.size = end - entry->addr;
+
+	region.start = cur_entry.addr;
+	region.size = cur_entry.size;
 
 	/* Give up if slot area array is full. */
 	while (slot_area_index < MAX_SLOT_AREA) {
@@ -489,7 +522,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
 		region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
 
 		/* Did we raise the address above this e820 region? */
-		if (region.start > entry->addr + entry->size)
+		if (region.start > cur_entry.addr + cur_entry.size)
 			return;
 
 		/* Reduce size by any delta from the original address. */
-- 
2.5.5

[PATCH v4 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description

[Baoquan He]

In commit:

  9710f581bb4c ("x86, mm: Let "memmap=" take more entries one time")

... 'memmap=' was changed to adopt multiple, comma delimited values in a
single entry, so update the related description.

In the special case of only specifying size value without an offset,
like memmap=nn[KMG], memmap behaves similarly to mem=nn[KMG], so update
it too here.

Furthermore, for memmap=nn[KMG]$ss[KMG], an escape character needs be added
before '$' for some bootloaders. E.g in grub2, if we specify memmap=100M$5G
as suggested by the documentation, "memmap=100MG" gets passed to the kernel.

Clarify all this.

Signed-off-by: Baoquan He <bhe@redhat.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 e4c9e0e..8d37f26 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2112,6 +2112,12 @@
 	memmap=nn[KMG]@ss[KMG]
 			[KNL] Force usage of a specific region of memory.
 			Region of memory to be used is from ss to ss+nn.
+			If @ss[KMG] is omitted, it equals to mem=nn[KMG]
+			which limits max address as nn[KMG].
+			Multiple different options can be put into one entry
+			with comma delimited to save space:
+			Example:
+				memmap=100M@2G,100M#3G,1G!1024G
 
 	memmap=nn[KMG]#ss[KMG]
 			[KNL,ACPI] Mark specific memory as ACPI data.
@@ -2124,6 +2130,9 @@
 			         memmap=64K$0x18690000
 			         or
 			         memmap=0x10000$0x18690000
+			Some bootloaders may need escape character before '$',
+			like in grub2, otherwise '$' and the following number
+			will be eaten.
 
 	memmap=nn[KMG]!ss[KMG]
 			[KNL,X86] Mark specific memory as protected.
-- 
2.5.5

Re: [PATCH v4 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Kees Cook]

On Mon, May 8, 2017 at 10:57 PM, Baoquan He <bhe@redhat.com> wrote:
> Option mem= will limit the max address a system can use and any memory
> region above the limit will be removed.
>
> Furthermore, memmap=nn[KMG] which has no offset specified has the same
> behaviour as mem=.
>
> KASLR needs to consider this when choosing the random position for
> decompressing the kernel.
>
> This patch implements that.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 69 +++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 106e13b..0637e85 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -88,6 +88,10 @@ struct mem_vector {
>  static bool memmap_too_large;
>
>
> +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> +unsigned long long mem_limit = ULLONG_MAX;
> +
> +
>  enum mem_avoid_index {
>         MEM_AVOID_ZO_RANGE = 0,
>         MEM_AVOID_INITRD,
> @@ -138,16 +142,24 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>                 return -EINVAL;
>
>         switch (*p) {
> -       case '@':
> -               /* Skip this region, usable */
> -               *start = 0;
> -               *size = 0;
> -               return 0;
>         case '#':
>         case '$':
>         case '!':
>                 *start = memparse(p + 1, &p);
>                 return 0;
> +       case '@':       /* Fall through: */
> +               /*
> +                * memmap=nn@ss specifies usable region, should be skipped
> +                */
> +               *size = 0;

The "fall through" comment should go here (where the break is "missing").

> +       default:
> +               /*
> +                * If w/o offset, only size specified, memmap=nn[KMG] has the
> +                * same behaviour as mem=nn[KMG]. It limits the max address
> +                * system can use. Region above the limit should be avoided.
> +                */
> +               *start = 0;
> +               return 0;
>         }
>
>         return -EINVAL;
> @@ -173,9 +185,14 @@ static void mem_avoid_memmap(char *str)
>                 if (rc < 0)
>                         break;
>                 str = k;
> -               /* A usable region that should not be skipped */
> -               if (size == 0)
> +
> +               if (start == 0) {
> +                       /* Store the specified memory limit if size > 0 */
> +                       if (size > 0)
> +                               mem_limit = size;
> +
>                         continue;
> +               }
>
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> @@ -187,19 +204,15 @@ static void mem_avoid_memmap(char *str)
>                 memmap_too_large = true;
>  }
>
> -
> -/*
> - * handle_mem_memmap will also cover 'mem=' issue in next patch. Will remove
> - * this note later.
> - */
>  static int handle_mem_memmap(void)
>  {
>         char *args = (char *)get_cmd_line_ptr();
>         size_t len = strlen((char *)args);
>         char *tmp_cmdline;
>         char *param, *val;
> +       u64 mem_size;
>
> -       if (!strstr(args, "memmap="))
> +       if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>                 return 0;
>
>         tmp_cmdline = malloc(len + 1);
> @@ -222,8 +235,20 @@ static int handle_mem_memmap(void)
>                         return -1;
>                 }
>
> -               if (!strcmp(param, "memmap"))
> +               if (!strcmp(param, "memmap")) {
>                         mem_avoid_memmap(val);
> +               } else if (!strcmp(param, "mem")) {
> +                       char *p = val;
> +
> +                       if (!strcmp(p, "nopentium"))
> +                               continue;
> +                       mem_size = memparse(p, &p);
> +                       if (mem_size == 0) {
> +                               free(tmp_cmdline);
> +                               return -EINVAL;
> +                       }
> +                       mem_limit = mem_size;
> +               }
>         }
>
>         free(tmp_cmdline);
> @@ -460,7 +485,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>  {
>         struct mem_vector region, overlap;
>         struct slot_area slot_area;
> -       unsigned long start_orig;
> +       unsigned long start_orig, end;
> +       struct boot_e820_entry cur_entry;
>
>         /* Skip non-RAM entries. */
>         if (entry->type != E820_TYPE_RAM)
> @@ -474,8 +500,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>         if (entry->addr + entry->size < minimum)
>                 return;
>
> -       region.start = entry->addr;
> -       region.size = entry->size;
> +       /* Ignore entries above memory limit */
> +       end = min(entry->size + entry->addr, mem_limit);
> +       if (entry->addr >= end)
> +               return;
> +       cur_entry.addr = entry->addr;
> +       cur_entry.size = end - entry->addr;
> +
> +       region.start = cur_entry.addr;
> +       region.size = cur_entry.size;
>
>         /* Give up if slot area array is full. */
>         while (slot_area_index < MAX_SLOT_AREA) {
> @@ -489,7 +522,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>                 region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
>
>                 /* Did we raise the address above this e820 region? */
> -               if (region.start > entry->addr + entry->size)
> +               if (region.start > cur_entry.addr + cur_entry.size)
>                         return;
>
>                 /* Reduce size by any delta from the original address. */
> --
> 2.5.5
>

Otherwise looks fine to me!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Baoquan He]

On 05/09/17 at 10:39am, Kees Cook wrote:
> On Mon, May 8, 2017 at 10:57 PM, Baoquan He <bhe@redhat.com> wrote:
> > Option mem= will limit the max address a system can use and any memory
> > region above the limit will be removed.
> >
> > Furthermore, memmap=nn[KMG] which has no offset specified has the same
> > behaviour as mem=.
> >
> > KASLR needs to consider this when choosing the random position for
> > decompressing the kernel.
> >
> > This patch implements that.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/kaslr.c | 69 +++++++++++++++++++++++++++++-----------
> >  1 file changed, 51 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 106e13b..0637e85 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -88,6 +88,10 @@ struct mem_vector {
> >  static bool memmap_too_large;
> >
> >
> > +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> > +unsigned long long mem_limit = ULLONG_MAX;
> > +
> > +
> >  enum mem_avoid_index {
> >         MEM_AVOID_ZO_RANGE = 0,
> >         MEM_AVOID_INITRD,
> > @@ -138,16 +142,24 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >                 return -EINVAL;
> >
> >         switch (*p) {
> > -       case '@':
> > -               /* Skip this region, usable */
> > -               *start = 0;
> > -               *size = 0;
> > -               return 0;
> >         case '#':
> >         case '$':
> >         case '!':
> >                 *start = memparse(p + 1, &p);
> >                 return 0;
> > +       case '@':       /* Fall through: */
> > +               /*
> > +                * memmap=nn@ss specifies usable region, should be skipped
> > +                */
> > +               *size = 0;
> 
> The "fall through" comment should go here (where the break is "missing").

Ah, OK, will change and repost after patch 1/3 reviewing. Thanks a lot!

> 
> > +       default:
> > +               /*
> > +                * If w/o offset, only size specified, memmap=nn[KMG] has the
> > +                * same behaviour as mem=nn[KMG]. It limits the max address
> > +                * system can use. Region above the limit should be avoided.
> > +                */
> > +               *start = 0;
> > +               return 0;
> >         }
> >
> >         return -EINVAL;
> > @@ -173,9 +185,14 @@ static void mem_avoid_memmap(char *str)
> >                 if (rc < 0)
> >                         break;
> >                 str = k;
> > -               /* A usable region that should not be skipped */
> > -               if (size == 0)
> > +
> > +               if (start == 0) {
> > +                       /* Store the specified memory limit if size > 0 */
> > +                       if (size > 0)
> > +                               mem_limit = size;
> > +
> >                         continue;
> > +               }
> >
> >                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
> >                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> > @@ -187,19 +204,15 @@ static void mem_avoid_memmap(char *str)
> >                 memmap_too_large = true;
> >  }
> >
> > -
> > -/*
> > - * handle_mem_memmap will also cover 'mem=' issue in next patch. Will remove
> > - * this note later.
> > - */
> >  static int handle_mem_memmap(void)
> >  {
> >         char *args = (char *)get_cmd_line_ptr();
> >         size_t len = strlen((char *)args);
> >         char *tmp_cmdline;
> >         char *param, *val;
> > +       u64 mem_size;
> >
> > -       if (!strstr(args, "memmap="))
> > +       if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> >                 return 0;
> >
> >         tmp_cmdline = malloc(len + 1);
> > @@ -222,8 +235,20 @@ static int handle_mem_memmap(void)
> >                         return -1;
> >                 }
> >
> > -               if (!strcmp(param, "memmap"))
> > +               if (!strcmp(param, "memmap")) {
> >                         mem_avoid_memmap(val);
> > +               } else if (!strcmp(param, "mem")) {
> > +                       char *p = val;
> > +
> > +                       if (!strcmp(p, "nopentium"))
> > +                               continue;
> > +                       mem_size = memparse(p, &p);
> > +                       if (mem_size == 0) {
> > +                               free(tmp_cmdline);
> > +                               return -EINVAL;
> > +                       }
> > +                       mem_limit = mem_size;
> > +               }
> >         }
> >
> >         free(tmp_cmdline);
> > @@ -460,7 +485,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> >  {
> >         struct mem_vector region, overlap;
> >         struct slot_area slot_area;
> > -       unsigned long start_orig;
> > +       unsigned long start_orig, end;
> > +       struct boot_e820_entry cur_entry;
> >
> >         /* Skip non-RAM entries. */
> >         if (entry->type != E820_TYPE_RAM)
> > @@ -474,8 +500,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> >         if (entry->addr + entry->size < minimum)
> >                 return;
> >
> > -       region.start = entry->addr;
> > -       region.size = entry->size;
> > +       /* Ignore entries above memory limit */
> > +       end = min(entry->size + entry->addr, mem_limit);
> > +       if (entry->addr >= end)
> > +               return;
> > +       cur_entry.addr = entry->addr;
> > +       cur_entry.size = end - entry->addr;
> > +
> > +       region.start = cur_entry.addr;
> > +       region.size = cur_entry.size;
> >
> >         /* Give up if slot area array is full. */
> >         while (slot_area_index < MAX_SLOT_AREA) {
> > @@ -489,7 +522,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> >                 region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
> >
> >                 /* Did we raise the address above this e820 region? */
> > -               if (region.start > entry->addr + entry->size)
> > +               if (region.start > cur_entry.addr + cur_entry.size)
> >                         return;
> >
> >                 /* Reduce size by any delta from the original address. */
> > --
> > 2.5.5
> >
> 
> Otherwise looks fine to me!
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Re: [PATCH v4 1/3] KASLR: Parse all memmap entries in cmdline

[Thomas Gleixner]

On Tue, 9 May 2017, Baoquan He wrote:

> In commit:
> 
>   f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")
> 
> ... the memmap= option is parsed so that KASLR can avoid those reserved
> regions. It uses cmdline_find_option() to get the value if memmap=
> is specified, however the problem is that cmdline_find_option() can only
> find the last entry if multiple memmap entries are provided. This
> is not correct.
> 
> In this patch, the whole cmdline will be scanned to search each

Can you please finally stop using this 'This patch does foo', 'In this
patch' phrases. They are bogus. We already know that this is a patch
otherwise you wouldn't have sent it.

See Documentation/process/SubmittingPatches.txt

Aside of that can you please use properly written out words instead of
using random abbreviations in the changelog, e.g. command line instead of
cmdline?

> memmap, all of them will be parsed and handled.

A proper example would be:

   Address this by checking each command line token for a "memmap=" match
   and parse each instance instead of using cmdline_find_option().

Thanks,

	tglx

Re: [PATCH v4 1/3] KASLR: Parse all memmap entries in cmdline

[Baoquan He]

On 05/10/17 at 02:29pm, Thomas Gleixner wrote:
> On Tue, 9 May 2017, Baoquan He wrote:
> 
> > In commit:
> > 
> >   f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")
> > 
> > ... the memmap= option is parsed so that KASLR can avoid those reserved
> > regions. It uses cmdline_find_option() to get the value if memmap=
> > is specified, however the problem is that cmdline_find_option() can only
> > find the last entry if multiple memmap entries are provided. This
> > is not correct.
> > 
> > In this patch, the whole cmdline will be scanned to search each
> 
> Can you please finally stop using this 'This patch does foo', 'In this
> patch' phrases. They are bogus. We already know that this is a patch
> otherwise you wouldn't have sent it.
> 
> See Documentation/process/SubmittingPatches.txt
> 
> Aside of that can you please use properly written out words instead of
> using random abbreviations in the changelog, e.g. command line instead of
> cmdline?
> 
> > memmap, all of them will be parsed and handled.
> 
> A proper example would be:
> 
>    Address this by checking each command line token for a "memmap=" match
>    and parse each instance instead of using cmdline_find_option().

Sorry for those mistakes. Will change accordingly when repost.

Thanks
Baoquan

Re: [PATCH v4 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Masayoshi Mizuma]

Hi Baoquan,

Thank you for fixing!
I confirmed that the kernel is located within mem= address.

Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Regards,
Masayoshi Mizuma

On Tue, 9 May 2017 13:57:51 +0800 Baoquan He wrote:

> Option mem= will limit the max address a system can use and any memory
> region above the limit will be removed.
> 
> Furthermore, memmap=nn[KMG] which has no offset specified has the same
> behaviour as mem=.
> 
> KASLR needs to consider this when choosing the random position for
> decompressing the kernel.
> 
> This patch implements that.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 69 +++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 106e13b..0637e85 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -88,6 +88,10 @@ struct mem_vector {
>  static bool memmap_too_large;
>  
>  
> +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> +unsigned long long mem_limit = ULLONG_MAX;
> +
> +
>  enum mem_avoid_index {
>  	MEM_AVOID_ZO_RANGE = 0,
>  	MEM_AVOID_INITRD,
> @@ -138,16 +142,24 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>  		return -EINVAL;
>  
>  	switch (*p) {
> -	case '@':
> -		/* Skip this region, usable */
> -		*start = 0;
> -		*size = 0;
> -		return 0;
>  	case '#':
>  	case '$':
>  	case '!':
>  		*start = memparse(p + 1, &p);
>  		return 0;
> +	case '@':	/* Fall through: */
> +		/*
> +		 * memmap=nn@ss specifies usable region, should be skipped
> +		 */
> +		*size = 0;
> +	default:
> +		/*
> +		 * If w/o offset, only size specified, memmap=nn[KMG] has the
> +		 * same behaviour as mem=nn[KMG]. It limits the max address
> +		 * system can use. Region above the limit should be avoided.
> +		 */
> +		*start = 0;
> +		return 0;
>  	}
>  
>  	return -EINVAL;
> @@ -173,9 +185,14 @@ static void mem_avoid_memmap(char *str)
>  		if (rc < 0)
>  			break;
>  		str = k;
> -		/* A usable region that should not be skipped */
> -		if (size == 0)
> +
> +		if (start == 0) {
> +			/* Store the specified memory limit if size > 0 */
> +			if (size > 0)
> +				mem_limit = size;
> +
>  			continue;
> +		}
>  
>  		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
>  		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> @@ -187,19 +204,15 @@ static void mem_avoid_memmap(char *str)
>  		memmap_too_large = true;
>  }
>  
> -
> -/*
> - * handle_mem_memmap will also cover 'mem=' issue in next patch. Will remove
> - * this note later.
> - */
>  static int handle_mem_memmap(void)
>  {
>  	char *args = (char *)get_cmd_line_ptr();
>  	size_t len = strlen((char *)args);
>  	char *tmp_cmdline;
>  	char *param, *val;
> +	u64 mem_size;
>  
> -	if (!strstr(args, "memmap="))
> +	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
>  		return 0;
>  
>  	tmp_cmdline = malloc(len + 1);
> @@ -222,8 +235,20 @@ static int handle_mem_memmap(void)
>  			return -1;
>  		}
>  
> -		if (!strcmp(param, "memmap"))
> +		if (!strcmp(param, "memmap")) {
>  			mem_avoid_memmap(val);
> +		} else if (!strcmp(param, "mem")) {
> +			char *p = val;
> +
> +			if (!strcmp(p, "nopentium"))
> +				continue;
> +			mem_size = memparse(p, &p);
> +			if (mem_size == 0) {
> +				free(tmp_cmdline);
> +				return -EINVAL;
> +			}
> +			mem_limit = mem_size;
> +		}
>  	}
>  
>  	free(tmp_cmdline);
> @@ -460,7 +485,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>  {
>  	struct mem_vector region, overlap;
>  	struct slot_area slot_area;
> -	unsigned long start_orig;
> +	unsigned long start_orig, end;
> +	struct boot_e820_entry cur_entry;
>  
>  	/* Skip non-RAM entries. */
>  	if (entry->type != E820_TYPE_RAM)
> @@ -474,8 +500,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>  	if (entry->addr + entry->size < minimum)
>  		return;
>  
> -	region.start = entry->addr;
> -	region.size = entry->size;
> +	/* Ignore entries above memory limit */
> +	end = min(entry->size + entry->addr, mem_limit);
> +	if (entry->addr >= end)
> +		return;
> +	cur_entry.addr = entry->addr;
> +	cur_entry.size = end - entry->addr;
> +
> +	region.start = cur_entry.addr;
> +	region.size = cur_entry.size;
>  
>  	/* Give up if slot area array is full. */
>  	while (slot_area_index < MAX_SLOT_AREA) {
> @@ -489,7 +522,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>  		region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
>  
>  		/* Did we raise the address above this e820 region? */
> -		if (region.start > entry->addr + entry->size)
> +		if (region.start > cur_entry.addr + cur_entry.size)
>  			return;
>  
>  		/* Reduce size by any delta from the original address. */
> -- 
> 2.5.5
> 

Re: [PATCH v4 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Baoquan He]

On 05/12/17 at 01:15pm, Masayoshi Mizuma wrote:
> Hi Baoquan,
> 
> Thank you for fixing!
> I confirmed that the kernel is located within mem= address.
> 
> Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Thanks for your testing!

I will post v5 according to comments from Thomas and Kees. Since no
functionality code change, will add your Tested-by.

Thanks
Baoquan

> 
> On Tue, 9 May 2017 13:57:51 +0800 Baoquan He wrote:
> 
> > Option mem= will limit the max address a system can use and any memory
> > region above the limit will be removed.
> > 
> > Furthermore, memmap=nn[KMG] which has no offset specified has the same
> > behaviour as mem=.
> > 
> > KASLR needs to consider this when choosing the random position for
> > decompressing the kernel.
> > 
> > This patch implements that.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/kaslr.c | 69 +++++++++++++++++++++++++++++-----------
> >  1 file changed, 51 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 106e13b..0637e85 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -88,6 +88,10 @@ struct mem_vector {
> >  static bool memmap_too_large;
> >  
> >  
> > +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> > +unsigned long long mem_limit = ULLONG_MAX;
> > +
> > +
> >  enum mem_avoid_index {
> >  	MEM_AVOID_ZO_RANGE = 0,
> >  	MEM_AVOID_INITRD,
> > @@ -138,16 +142,24 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >  		return -EINVAL;
> >  
> >  	switch (*p) {
> > -	case '@':
> > -		/* Skip this region, usable */
> > -		*start = 0;
> > -		*size = 0;
> > -		return 0;
> >  	case '#':
> >  	case '$':
> >  	case '!':
> >  		*start = memparse(p + 1, &p);
> >  		return 0;
> > +	case '@':	/* Fall through: */
> > +		/*
> > +		 * memmap=nn@ss specifies usable region, should be skipped
> > +		 */
> > +		*size = 0;
> > +	default:
> > +		/*
> > +		 * If w/o offset, only size specified, memmap=nn[KMG] has the
> > +		 * same behaviour as mem=nn[KMG]. It limits the max address
> > +		 * system can use. Region above the limit should be avoided.
> > +		 */
> > +		*start = 0;
> > +		return 0;
> >  	}
> >  
> >  	return -EINVAL;
> > @@ -173,9 +185,14 @@ static void mem_avoid_memmap(char *str)
> >  		if (rc < 0)
> >  			break;
> >  		str = k;
> > -		/* A usable region that should not be skipped */
> > -		if (size == 0)
> > +
> > +		if (start == 0) {
> > +			/* Store the specified memory limit if size > 0 */
> > +			if (size > 0)
> > +				mem_limit = size;
> > +
> >  			continue;
> > +		}
> >  
> >  		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
> >  		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> > @@ -187,19 +204,15 @@ static void mem_avoid_memmap(char *str)
> >  		memmap_too_large = true;
> >  }
> >  
> > -
> > -/*
> > - * handle_mem_memmap will also cover 'mem=' issue in next patch. Will remove
> > - * this note later.
> > - */
> >  static int handle_mem_memmap(void)
> >  {
> >  	char *args = (char *)get_cmd_line_ptr();
> >  	size_t len = strlen((char *)args);
> >  	char *tmp_cmdline;
> >  	char *param, *val;
> > +	u64 mem_size;
> >  
> > -	if (!strstr(args, "memmap="))
> > +	if (!strstr(args, "memmap=") && !strstr(args, "mem="))
> >  		return 0;
> >  
> >  	tmp_cmdline = malloc(len + 1);
> > @@ -222,8 +235,20 @@ static int handle_mem_memmap(void)
> >  			return -1;
> >  		}
> >  
> > -		if (!strcmp(param, "memmap"))
> > +		if (!strcmp(param, "memmap")) {
> >  			mem_avoid_memmap(val);
> > +		} else if (!strcmp(param, "mem")) {
> > +			char *p = val;
> > +
> > +			if (!strcmp(p, "nopentium"))
> > +				continue;
> > +			mem_size = memparse(p, &p);
> > +			if (mem_size == 0) {
> > +				free(tmp_cmdline);
> > +				return -EINVAL;
> > +			}
> > +			mem_limit = mem_size;
> > +		}
> >  	}
> >  
> >  	free(tmp_cmdline);
> > @@ -460,7 +485,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> >  {
> >  	struct mem_vector region, overlap;
> >  	struct slot_area slot_area;
> > -	unsigned long start_orig;
> > +	unsigned long start_orig, end;
> > +	struct boot_e820_entry cur_entry;
> >  
> >  	/* Skip non-RAM entries. */
> >  	if (entry->type != E820_TYPE_RAM)
> > @@ -474,8 +500,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> >  	if (entry->addr + entry->size < minimum)
> >  		return;
> >  
> > -	region.start = entry->addr;
> > -	region.size = entry->size;
> > +	/* Ignore entries above memory limit */
> > +	end = min(entry->size + entry->addr, mem_limit);
> > +	if (entry->addr >= end)
> > +		return;
> > +	cur_entry.addr = entry->addr;
> > +	cur_entry.size = end - entry->addr;
> > +
> > +	region.start = cur_entry.addr;
> > +	region.size = cur_entry.size;
> >  
> >  	/* Give up if slot area array is full. */
> >  	while (slot_area_index < MAX_SLOT_AREA) {
> > @@ -489,7 +522,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> >  		region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
> >  
> >  		/* Did we raise the address above this e820 region? */
> > -		if (region.start > entry->addr + entry->size)
> > +		if (region.start > cur_entry.addr + cur_entry.size)
> >  			return;
> >  
> >  		/* Reduce size by any delta from the original address. */
> > -- 
> > 2.5.5
> > 
>