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

[PATCH 0/4] 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.

Baoquan He (4):
  param: Move function next_arg to lib/cmdline.c for later reuse
  KASLR: Parse all memmap entries in cmdline
  KASLR: Handle memory limit specified by memmap and mem option
  doc: Update description about memmap option in kernel-parameter.txt

 Documentation/admin-guide/kernel-parameters.txt |   9 ++
 arch/x86/boot/compressed/cmdline.c              |   2 +-
 arch/x86/boot/compressed/kaslr.c                | 161 ++++++++++++++----------
 arch/x86/boot/string.c                          |   8 ++
 include/linux/kernel.h                          |   1 +
 kernel/params.c                                 |  52 --------
 lib/cmdline.c                                   |  57 +++++++++
 7 files changed, 172 insertions(+), 118 deletions(-)

-- 
2.5.5

[PATCH 1/4] param: Move function next_arg to lib/cmdline.c for later reuse

[Baoquan He]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=n, Size: 4299 bytes --]

next_arg will be used to parse cmdline in x86/boot/compressed code,
so move it to lib/cmdline.c for better code reuse.

No change in functionality.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jessica Yu <jeyu@redhat.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: zijun_hu <zijun_hu@htc.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/kernel.h |  1 +
 kernel/params.c        | 52 ---------------------------------------------
 lib/cmdline.c          | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3..7ae2567 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -438,6 +438,7 @@ extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
 extern bool parse_option_str(const char *str, const char *option);
+extern char *next_arg(char *args, char **param, char **val);
 
 extern int core_kernel_text(unsigned long addr);
 extern int core_kernel_data(unsigned long addr);
diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..60b2d81 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -160,58 +160,6 @@ static int parse_one(char *param,
 	return -ENOENT;
 }
 
-/* You can use " around spaces, but can't escape ". */
-/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
-{
-	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
-	char *next;
-
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
-	}
-
-	for (i = 0; args[i]; i++) {
-		if (isspace(args[i]) && !in_quote)
-			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
-		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
-	}
-
-	*param = args;
-	if (!equals)
-		*val = NULL;
-	else {
-		args[equals] = '\0';
-		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
-		}
-	}
-	if (quoted && args[i-1] == '"')
-		args[i-1] = '\0';
-
-	if (args[i]) {
-		args[i] = '\0';
-		next = args + i + 1;
-	} else
-		next = args + i;
-
-	/* Chew up trailing spaces. */
-	return skip_spaces(next);
-}
-
 /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
 char *parse_args(const char *doing,
 		 char *args,
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 8f13cf7..6e3abfb 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -15,6 +15,7 @@
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 
 /*
  *	If a hyphen was found in get_option, this will handle the
@@ -189,3 +190,59 @@ bool parse_option_str(const char *str, const char *option)
 
 	return false;
 }
+
+/*
+ * Parse a string to get a param value pair.
+ * You can use " around spaces, but can't escape ".
+ * Hyphens and underscores equivalent in parameter names.
+ */
+char *next_arg(char *args, char **param, char **val)
+{
+	unsigned int i, equals = 0;
+	int in_quote = 0, quoted = 0;
+	char *next;
+
+	if (*args == '"') {
+		args++;
+		in_quote = 1;
+		quoted = 1;
+	}
+
+	for (i = 0; args[i]; i++) {
+		if (isspace(args[i]) && !in_quote)
+			break;
+		if (equals == 0) {
+			if (args[i] == '=')
+				equals = i;
+		}
+		if (args[i] == '"')
+			in_quote = !in_quote;
+	}
+
+	*param = args;
+	if (!equals)
+		*val = NULL;
+	else {
+		args[equals] = '\0';
+		*val = args + equals + 1;
+
+		/* Don't include quotes in value. */
+		if (**val == '"') {
+			(*val)++;
+			if (args[i-1] == '"')
+				args[i-1] = '\0';
+		}
+	}
+	if (quoted && args[i-1] == '"')
+		args[i-1] = '\0';
+
+	if (args[i]) {
+		args[i] = '\0';
+		next = args + i + 1;
+	} else
+		next = args + i;
+
+	/* Chew up trailing spaces. */
+	return skip_spaces(next);
+	//return next;
+}
-- 
2.5.5

[PATCH 2/4] KASLR: Parse all memmap entries in cmdline

[Baoquan He]

In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"),
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 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>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Kees Cook <keescook@chromium.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
---
 arch/x86/boot/compressed/cmdline.c |   2 +-
 arch/x86/boot/compressed/kaslr.c   | 112 ++++++++++++++++++-------------------
 arch/x86/boot/string.c             |   8 +++
 3 files changed, 65 insertions(+), 57 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 8b7c9e7..36ab429 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -9,14 +9,16 @@
  * contain the entire properly aligned running kernel image.
  *
  */
+
+#define BOOT_CTYPE_H
 #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>
 
 /* Simplified build-specific string for starting entropy. */
@@ -61,6 +63,9 @@ struct mem_vector {
 #define MAX_MEMMAP_REGIONS	4
 
 static bool memmap_too_large;
+int mem_avoid_memmap_index;
+extern unsigned long get_cmd_line_ptr(void);
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -85,49 +90,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,40 +112,33 @@ 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;
 
 	switch (*p) {
 	case '@':
 		/* Skip this region, usable */
-		*start = 0;
 		*size = 0;
-		return 0;
+		*start = 0;
 	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];
 	int rc;
-	int i;
-	char *str;
+	int i = mem_avoid_memmap_index;
 
-	/* 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;
@@ -196,12 +159,49 @@ static void mem_avoid_memmap(void)
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
 		i++;
 	}
+	mem_avoid_memmap_index = i;
 
 	/* More than 4 memmaps, fail kaslr */
 	if ((i >= MAX_MEMMAP_REGIONS) && str)
 		memmap_too_large = true;
 }
 
+#define COMMAND_LINE_SIZE 256
+static int handle_mem_memmap(void)
+{
+	char *args = (char *)get_cmd_line_ptr();
+	char tmp_cmdline[COMMAND_LINE_SIZE];
+	size_t len = strlen((char *)args);
+	char *param, *val;
+
+	len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
+	memcpy(tmp_cmdline, args, len);
+	tmp_cmdline[len] = 0;
+	args = tmp_cmdline;
+
+	/* Chew leading spaces */
+	args = skip_spaces(args);
+
+	while (*args) {
+		int ret;
+
+		debug_putstr(args);
+		debug_putstr("\n");
+
+		args = next_arg(args, &param, &val);
+		/* Stop at -- */
+		if (!val && strcmp(param, "--") == 0) {
+			warn("Only '--' specified in cmdline");
+			return -1;
+		}
+
+		if (!strcmp(param, "memmap"))
+			mem_avoid_memmap(val);
+	}
+
+	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 +323,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 3/4] KASLR: Handle memory limit specified by memmap and mem option

[Baoquan He]

Option mem= will limit the max address system can use. Any memory
region above the limit will be removed. And memmap=nn[KMG] which
has no offset specified has the same behaviour as mem=. KASLR need
consider this when choose the random position for decompressing
kernel. Do it in this patch.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Cc: Kees Cook <keescook@chromium.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
---
 arch/x86/boot/compressed/kaslr.c | 53 +++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 36ab429..5361abd 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -67,6 +67,10 @@ int mem_avoid_memmap_index;
 extern unsigned long get_cmd_line_ptr(void);
 
 
+/* 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,
@@ -117,15 +121,18 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 		return -EINVAL;
 
 	switch (*p) {
-	case '@':
-		/* Skip this region, usable */
-		*size = 0;
-		*start = 0;
 	case '#':
 	case '$':
 	case '!':
 		*start = memparse(p + 1, &p);
 		return 0;
+	case '@':
+		/* Skip this region, usable */
+		*size = 0;
+	default:
+		/* Avoid the region which is above the amount limit */
+		*start = 0;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -151,9 +158,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;
@@ -173,6 +185,7 @@ static int handle_mem_memmap(void)
 	char tmp_cmdline[COMMAND_LINE_SIZE];
 	size_t len = strlen((char *)args);
 	char *param, *val;
+	u64 mem_size;
 
 	len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
 	memcpy(tmp_cmdline, args, len);
@@ -195,8 +208,18 @@ 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)
+				return -EINVAL;
+			mem_limit = mem_size;
+		}
 	}
 
 	return 0;
@@ -432,7 +455,8 @@ static void process_e820_entry(struct e820entry *entry,
 {
 	struct mem_vector region, overlap;
 	struct slot_area slot_area;
-	unsigned long start_orig;
+	unsigned long start_orig, end;
+	struct e820entry cur_entry;
 
 	/* Skip non-RAM entries. */
 	if (entry->type != E820_RAM)
@@ -446,8 +470,15 @@ static void process_e820_entry(struct e820entry *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 - 1, mem_limit);
+	if (entry->addr >= end)
+		return;
+	cur_entry.addr = entry->addr;
+	cur_entry.size = end - entry->addr + 1;
+
+	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) {
@@ -461,7 +492,7 @@ static void process_e820_entry(struct e820entry *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 4/4] doc: Update description about memmap option in kernel-parameter.txt

[Baoquan He]

In commit 9710f581bb4c ("x86, mm: Let "memmap=" take more entries one time")
memmap was changed to adopt multiple values with comma delimited in
one entry, so update the related description.

And if only specify size value without offset, like memmap=nn[KMG],
memmap behaves equal to mem=nn[KMG], update it too here.

And for memmap=nn[KMG]$ss[KMG], escape character need be added before
'$' for some bootloaders. E.g in grub2, if specified memmap=100M$5G,
actually it passed memmap=100MG to kernel.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-doc@vger.kernel.org
---
 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 facc20a..6789262 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 ommited, 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 0/4] Handle memmap and mem kernel options in boot stage kaslr

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> 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.
> 
> Baoquan He (4):
>   param: Move function next_arg to lib/cmdline.c for later reuse
>   KASLR: Parse all memmap entries in cmdline
>   KASLR: Handle memory limit specified by memmap and mem option
>   doc: Update description about memmap option in kernel-parameter.txt
> 
>  Documentation/admin-guide/kernel-parameters.txt |   9 ++
>  arch/x86/boot/compressed/cmdline.c              |   2 +-
>  arch/x86/boot/compressed/kaslr.c                | 161 ++++++++++++++----------
>  arch/x86/boot/string.c                          |   8 ++
>  include/linux/kernel.h                          |   1 +
>  kernel/params.c                                 |  52 --------
>  lib/cmdline.c                                   |  57 +++++++++
>  7 files changed, 172 insertions(+), 118 deletions(-)

I ported this series to tip:x86/boot (please post future versions against that), 
and beyond a trivial conflict with e820entry => e820_entry, it fails to 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

... which could be due to bad relocations, but I've not dug any further.

Please also pick up the fixed/rewritten changelogs from the git log below for the 
next version.

Thanks,

	Ingo

====================>

Documentation/kernel-parameters.txt: Update 'memmap=' option description

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.

----

KASLR: Handle memory limit specified by memmap and mem option

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.

----

KASLR: Parse all memmap entries in cmdline

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.

----

boot/param: Move next_arg() function to lib/cmdline.c for later reuse

next_arg() will be used to parse boot parameters in the x86/boot/compressed code,
so move it to lib/cmdline.c for better code reuse.

No change in functionality.

Re: [PATCH 0/4] Handle memmap and mem kernel options in boot stage kaslr

[Baoquan He]

On 04/18/17 at 11:47am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > 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.
> > 
> > Baoquan He (4):
> >   param: Move function next_arg to lib/cmdline.c for later reuse
> >   KASLR: Parse all memmap entries in cmdline
> >   KASLR: Handle memory limit specified by memmap and mem option
> >   doc: Update description about memmap option in kernel-parameter.txt
> > 
> >  Documentation/admin-guide/kernel-parameters.txt |   9 ++
> >  arch/x86/boot/compressed/cmdline.c              |   2 +-
> >  arch/x86/boot/compressed/kaslr.c                | 161 ++++++++++++++----------
> >  arch/x86/boot/string.c                          |   8 ++
> >  include/linux/kernel.h                          |   1 +
> >  kernel/params.c                                 |  52 --------
> >  lib/cmdline.c                                   |  57 +++++++++
> >  7 files changed, 172 insertions(+), 118 deletions(-)
> 
> I ported this series to tip:x86/boot (please post future versions against that), 
> and beyond a trivial conflict with e820entry => e820_entry, it fails to 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
> 
> ... which could be due to bad relocations, but I've not dug any further.

Thanks, Ingo!

I will find a x86_32 system to try allmodconfig.

> 
> Please also pick up the fixed/rewritten changelogs from the git log below for the 
> next version.

Will do, thanks a lot!

> 
> Thanks,
> 
> 	Ingo
> 
> ====================>
> 
> Documentation/kernel-parameters.txt: Update 'memmap=' option description
> 
> 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.
> 
> ----
> 
> KASLR: Handle memory limit specified by memmap and mem option
> 
> 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.
> 
> ----
> 
> KASLR: Parse all memmap entries in cmdline
> 
> 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.
> 
> ----
> 
> boot/param: Move next_arg() function to lib/cmdline.c for later reuse
> 
> next_arg() will be used to parse boot parameters in the x86/boot/compressed code,
> so move it to lib/cmdline.c for better code reuse.
> 
> No change in functionality.
> 

Re: [PATCH 0/4] Handle memmap and mem kernel options in boot stage kaslr

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> On 04/18/17 at 11:47am, Ingo Molnar wrote:
> > 
> > * Baoquan He <bhe@redhat.com> wrote:
> > 
> > > 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.
> > > 
> > > Baoquan He (4):
> > >   param: Move function next_arg to lib/cmdline.c for later reuse
> > >   KASLR: Parse all memmap entries in cmdline
> > >   KASLR: Handle memory limit specified by memmap and mem option
> > >   doc: Update description about memmap option in kernel-parameter.txt
> > > 
> > >  Documentation/admin-guide/kernel-parameters.txt |   9 ++
> > >  arch/x86/boot/compressed/cmdline.c              |   2 +-
> > >  arch/x86/boot/compressed/kaslr.c                | 161 ++++++++++++++----------
> > >  arch/x86/boot/string.c                          |   8 ++
> > >  include/linux/kernel.h                          |   1 +
> > >  kernel/params.c                                 |  52 --------
> > >  lib/cmdline.c                                   |  57 +++++++++
> > >  7 files changed, 172 insertions(+), 118 deletions(-)
> > 
> > I ported this series to tip:x86/boot (please post future versions against that), 
> > and beyond a trivial conflict with e820entry => e820_entry, it fails to 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
> > 
> > ... which could be due to bad relocations, but I've not dug any further.
> 
> Thanks, Ingo!
> 
> I will find a x86_32 system to try allmodconfig.

No need, on a 64-bit system just do:

	make ARCH=i386 allmodconfig

and build the kernel the regular way.

Thanks,

	Ingo

[tip:x86/boot] boot/param: Move next_arg() function to lib/cmdline.c for later reuse

[tip-bot for Baoquan He]

Commit-ID:  f51b17c8d90f85456579c3192ab59ee031835634
Gitweb:     http://git.kernel.org/tip/f51b17c8d90f85456579c3192ab59ee031835634
Author:     Baoquan He <bhe@redhat.com>
AuthorDate: Mon, 17 Apr 2017 21:34:56 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 18 Apr 2017 10:37:13 +0200

boot/param: Move next_arg() function to lib/cmdline.c for later reuse

next_arg() will be used to parse boot parameters in the x86/boot/compressed code,
so move it to lib/cmdline.c for better code reuse.

No change in functionality.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Jens Axboe <axboe@fb.com>
Cc: Jessica Yu <jeyu@redhat.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dan.j.williams@intel.com
Cc: dave.jiang@intel.com
Cc: dyoung@redhat.com
Cc: keescook@chromium.org
Cc: zijun_hu <zijun_hu@htc.com>
Link: http://lkml.kernel.org/r/1492436099-4017-2-git-send-email-bhe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/kernel.h |  1 +
 kernel/params.c        | 52 ---------------------------------------------
 lib/cmdline.c          | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3..7ae2567 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -438,6 +438,7 @@ extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
 extern bool parse_option_str(const char *str, const char *option);
+extern char *next_arg(char *args, char **param, char **val);
 
 extern int core_kernel_text(unsigned long addr);
 extern int core_kernel_data(unsigned long addr);
diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..60b2d81 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -160,58 +160,6 @@ static int parse_one(char *param,
 	return -ENOENT;
 }
 
-/* You can use " around spaces, but can't escape ". */
-/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
-{
-	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
-	char *next;
-
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
-	}
-
-	for (i = 0; args[i]; i++) {
-		if (isspace(args[i]) && !in_quote)
-			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
-		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
-	}
-
-	*param = args;
-	if (!equals)
-		*val = NULL;
-	else {
-		args[equals] = '\0';
-		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
-		}
-	}
-	if (quoted && args[i-1] == '"')
-		args[i-1] = '\0';
-
-	if (args[i]) {
-		args[i] = '\0';
-		next = args + i + 1;
-	} else
-		next = args + i;
-
-	/* Chew up trailing spaces. */
-	return skip_spaces(next);
-}
-
 /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
 char *parse_args(const char *doing,
 		 char *args,
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 8f13cf7..3c6432df 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -15,6 +15,7 @@
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 
 /*
  *	If a hyphen was found in get_option, this will handle the
@@ -189,3 +190,59 @@ bool parse_option_str(const char *str, const char *option)
 
 	return false;
 }
+
+/*
+ * Parse a string to get a param value pair.
+ * You can use " around spaces, but can't escape ".
+ * Hyphens and underscores equivalent in parameter names.
+ */
+char *next_arg(char *args, char **param, char **val)
+{
+	unsigned int i, equals = 0;
+	int in_quote = 0, quoted = 0;
+	char *next;
+
+	if (*args == '"') {
+		args++;
+		in_quote = 1;
+		quoted = 1;
+	}
+
+	for (i = 0; args[i]; i++) {
+		if (isspace(args[i]) && !in_quote)
+			break;
+		if (equals == 0) {
+			if (args[i] == '=')
+				equals = i;
+		}
+		if (args[i] == '"')
+			in_quote = !in_quote;
+	}
+
+	*param = args;
+	if (!equals)
+		*val = NULL;
+	else {
+		args[equals] = '\0';
+		*val = args + equals + 1;
+
+		/* Don't include quotes in value. */
+		if (**val == '"') {
+			(*val)++;
+			if (args[i-1] == '"')
+				args[i-1] = '\0';
+		}
+	}
+	if (quoted && args[i-1] == '"')
+		args[i-1] = '\0';
+
+	if (args[i]) {
+		args[i] = '\0';
+		next = args + i + 1;
+	} else
+		next = args + i;
+
+	/* Chew up trailing spaces. */
+	return skip_spaces(next);
+	//return next;
+}

Re: [PATCH 1/4] param: Move function next_arg to lib/cmdline.c for later reuse

[Kees Cook]

On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> next_arg will be used to parse cmdline in x86/boot/compressed code,
> so move it to lib/cmdline.c for better code reuse.
>
> No change in functionality.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jessica Yu <jeyu@redhat.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: zijun_hu <zijun_hu@htc.com>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
> Cc: Peter Zijlstra <peterz@infradead.org>

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

-Kees

> ---
>  include/linux/kernel.h |  1 +
>  kernel/params.c        | 52 ---------------------------------------------
>  lib/cmdline.c          | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4c26dc3..7ae2567 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -438,6 +438,7 @@ extern int get_option(char **str, int *pint);
>  extern char *get_options(const char *str, int nints, int *ints);
>  extern unsigned long long memparse(const char *ptr, char **retptr);
>  extern bool parse_option_str(const char *str, const char *option);
> +extern char *next_arg(char *args, char **param, char **val);
>
>  extern int core_kernel_text(unsigned long addr);
>  extern int core_kernel_data(unsigned long addr);
> diff --git a/kernel/params.c b/kernel/params.c
> index a6d6149..60b2d81 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -160,58 +160,6 @@ static int parse_one(char *param,
>         return -ENOENT;
>  }
>
> -/* You can use " around spaces, but can't escape ". */
> -/* Hyphens and underscores equivalent in parameter names. */
> -static char *next_arg(char *args, char **param, char **val)
> -{
> -       unsigned int i, equals = 0;
> -       int in_quote = 0, quoted = 0;
> -       char *next;
> -
> -       if (*args == '"') {
> -               args++;
> -               in_quote = 1;
> -               quoted = 1;
> -       }
> -
> -       for (i = 0; args[i]; i++) {
> -               if (isspace(args[i]) && !in_quote)
> -                       break;
> -               if (equals == 0) {
> -                       if (args[i] == '=')
> -                               equals = i;
> -               }
> -               if (args[i] == '"')
> -                       in_quote = !in_quote;
> -       }
> -
> -       *param = args;
> -       if (!equals)
> -               *val = NULL;
> -       else {
> -               args[equals] = '\0';
> -               *val = args + equals + 1;
> -
> -               /* Don't include quotes in value. */
> -               if (**val == '"') {
> -                       (*val)++;
> -                       if (args[i-1] == '"')
> -                               args[i-1] = '\0';
> -               }
> -       }
> -       if (quoted && args[i-1] == '"')
> -               args[i-1] = '\0';
> -
> -       if (args[i]) {
> -               args[i] = '\0';
> -               next = args + i + 1;
> -       } else
> -               next = args + i;
> -
> -       /* Chew up trailing spaces. */
> -       return skip_spaces(next);
> -}
> -
>  /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
>  char *parse_args(const char *doing,
>                  char *args,
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 8f13cf7..6e3abfb 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -15,6 +15,7 @@
>  #include <linux/export.h>
>  #include <linux/kernel.h>
>  #include <linux/string.h>
> +#include <linux/ctype.h>
>
>  /*
>   *     If a hyphen was found in get_option, this will handle the
> @@ -189,3 +190,59 @@ bool parse_option_str(const char *str, const char *option)
>
>         return false;
>  }
> +
> +/*
> + * Parse a string to get a param value pair.
> + * You can use " around spaces, but can't escape ".
> + * Hyphens and underscores equivalent in parameter names.
> + */
> +char *next_arg(char *args, char **param, char **val)
> +{
> +       unsigned int i, equals = 0;
> +       int in_quote = 0, quoted = 0;
> +       char *next;
> +
> +       if (*args == '"') {
> +               args++;
> +               in_quote = 1;
> +               quoted = 1;
> +       }
> +
> +       for (i = 0; args[i]; i++) {
> +               if (isspace(args[i]) && !in_quote)
> +                       break;
> +               if (equals == 0) {
> +                       if (args[i] == '=')
> +                               equals = i;
> +               }
> +               if (args[i] == '"')
> +                       in_quote = !in_quote;
> +       }
> +
> +       *param = args;
> +       if (!equals)
> +               *val = NULL;
> +       else {
> +               args[equals] = '\0';
> +               *val = args + equals + 1;
> +
> +               /* Don't include quotes in value. */
> +               if (**val == '"') {
> +                       (*val)++;
> +                       if (args[i-1] == '"')
> +                               args[i-1] = '\0';
> +               }
> +       }
> +       if (quoted && args[i-1] == '"')
> +               args[i-1] = '\0';
> +
> +       if (args[i]) {
> +               args[i] = '\0';
> +               next = args + i + 1;
> +       } else
> +               next = args + i;
> +
> +       /* Chew up trailing spaces. */
> +       return skip_spaces(next);
> +       //return next;
> +}
> --
> 2.5.5
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline

[Kees Cook]

On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"),
> 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 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>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/boot/compressed/cmdline.c |   2 +-
>  arch/x86/boot/compressed/kaslr.c   | 112 ++++++++++++++++++-------------------
>  arch/x86/boot/string.c             |   8 +++
>  3 files changed, 65 insertions(+), 57 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 8b7c9e7..36ab429 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -9,14 +9,16 @@
>   * contain the entire properly aligned running kernel image.
>   *
>   */
> +
> +#define BOOT_CTYPE_H
>  #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>
>
>  /* Simplified build-specific string for starting entropy. */
> @@ -61,6 +63,9 @@ struct mem_vector {
>  #define MAX_MEMMAP_REGIONS     4
>
>  static bool memmap_too_large;
> +int mem_avoid_memmap_index;
> +extern unsigned long get_cmd_line_ptr(void);
> +
>
>  enum mem_avoid_index {
>         MEM_AVOID_ZO_RANGE = 0,
> @@ -85,49 +90,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,40 +112,33 @@ 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;
>
>         switch (*p) {
>         case '@':
>                 /* Skip this region, usable */
> -               *start = 0;
>                 *size = 0;
> -               return 0;
> +               *start = 0;

Is this intentionally falling through? If so, why assign *start at all?

>         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];
>         int rc;
> -       int i;
> -       char *str;
> +       int i = mem_avoid_memmap_index;
>
> -       /* 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;
> @@ -196,12 +159,49 @@ static void mem_avoid_memmap(void)
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
>                 i++;
>         }
> +       mem_avoid_memmap_index = i;
>
>         /* More than 4 memmaps, fail kaslr */
>         if ((i >= MAX_MEMMAP_REGIONS) && str)
>                 memmap_too_large = true;
>  }
>
> +#define COMMAND_LINE_SIZE 256
> +static int handle_mem_memmap(void)
> +{
> +       char *args = (char *)get_cmd_line_ptr();
> +       char tmp_cmdline[COMMAND_LINE_SIZE];

Can't this use a dynamic allocation instead of the 256 limit?

> +       size_t len = strlen((char *)args);
> +       char *param, *val;
> +
> +       len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
> +       memcpy(tmp_cmdline, args, len);
> +       tmp_cmdline[len] = 0;
> +       args = tmp_cmdline;
> +
> +       /* Chew leading spaces */
> +       args = skip_spaces(args);
> +
> +       while (*args) {
> +               int ret;
> +
> +               debug_putstr(args);
> +               debug_putstr("\n");

Are these accidentally left over?

> +
> +               args = next_arg(args, &param, &val);
> +               /* Stop at -- */
> +               if (!val && strcmp(param, "--") == 0) {
> +                       warn("Only '--' specified in cmdline");
> +                       return -1;
> +               }
> +
> +               if (!strcmp(param, "memmap"))
> +                       mem_avoid_memmap(val);
> +       }
> +
> +       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 +323,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
>

Otherwise, yeah, this looks sensible.

-Kees

-- 
Kees Cook
Pixel Security

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

[Kees Cook]

On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> Option mem= will limit the max address system can use. Any memory
> region above the limit will be removed. And memmap=nn[KMG] which
> has no offset specified has the same behaviour as mem=. KASLR need
> consider this when choose the random position for decompressing
> kernel. Do it in this patch.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/boot/compressed/kaslr.c | 53 +++++++++++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 36ab429..5361abd 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -67,6 +67,10 @@ int mem_avoid_memmap_index;
>  extern unsigned long get_cmd_line_ptr(void);
>
>
> +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> +unsigned long long mem_limit = ULLONG_MAX;

I would either make this 0 or ULLONG_MAX - 1 (see below).

> +
> +
>  enum mem_avoid_index {
>         MEM_AVOID_ZO_RANGE = 0,
>         MEM_AVOID_INITRD,
> @@ -117,15 +121,18 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>                 return -EINVAL;
>
>         switch (*p) {
> -       case '@':
> -               /* Skip this region, usable */
> -               *size = 0;
> -               *start = 0;
>         case '#':
>         case '$':
>         case '!':
>                 *start = memparse(p + 1, &p);
>                 return 0;
> +       case '@':
> +               /* Skip this region, usable */
> +               *size = 0;

Now it looks like we're intentionally falling through. A comment
should be included to indicate it.

> +       default:
> +               /* Avoid the region which is above the amount limit */
> +               *start = 0;
> +               return 0;
>         }
>
>         return -EINVAL;
> @@ -151,9 +158,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;
> @@ -173,6 +185,7 @@ static int handle_mem_memmap(void)
>         char tmp_cmdline[COMMAND_LINE_SIZE];
>         size_t len = strlen((char *)args);
>         char *param, *val;
> +       u64 mem_size;
>
>         len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
>         memcpy(tmp_cmdline, args, len);
> @@ -195,8 +208,18 @@ 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)
> +                               return -EINVAL;
> +                       mem_limit = mem_size;
> +               }
>         }
>
>         return 0;
> @@ -432,7 +455,8 @@ static void process_e820_entry(struct e820entry *entry,
>  {
>         struct mem_vector region, overlap;
>         struct slot_area slot_area;
> -       unsigned long start_orig;
> +       unsigned long start_orig, end;
> +       struct e820entry cur_entry;
>
>         /* Skip non-RAM entries. */
>         if (entry->type != E820_RAM)
> @@ -446,8 +470,15 @@ static void process_e820_entry(struct e820entry *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 - 1, mem_limit);
> +       if (entry->addr >= end)
> +               return;
> +       cur_entry.addr = entry->addr;
> +       cur_entry.size = end - entry->addr + 1;
> +
> +       region.start = cur_entry.addr;
> +       region.size = cur_entry.size;

I find the manipulation of entry->addr +/- 1 confusing; it should just
be mem_limit that is adjusted:

    end = min(entry->size + entry->addr, mem_limit + 1);

And maybe to avoid mem_limit being giant by default, maybe have "0" be special?

    cur_entry.addr = entry->addr;
    if (mem_limit) {
         unsigned long end = min(entry->size + entry->addr, mem_limit + 1);
         if (entry->addr > end)
             return;
         cur_entry.size = end - entry->addr;
    } else {
          cur_entry.size = entry->size;
    }

or something... and maybe move the whole thing earlier so other tests
that examine entry->size are checked with the new adjusted value.

-Kees

>
>         /* Give up if slot area array is full. */
>         while (slot_area_index < MAX_SLOT_AREA) {
> @@ -461,7 +492,7 @@ static void process_e820_entry(struct e820entry *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
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline

[Baoquan He]

Hi Kees,

Thanks for your reviewing!

On 04/18/17 at 01:22pm, Kees Cook wrote:

> >  static int
> >  parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > @@ -142,40 +112,33 @@ 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;
> >
> >         switch (*p) {
> >         case '@':
> >                 /* Skip this region, usable */
> > -               *start = 0;
> >                 *size = 0;
> > -               return 0;
> > +               *start = 0;
> 
> Is this intentionally falling through? If so, why assign *start at all?

OOPS, this is a mistake when I split patch. Here it should not be
changed in this patch though code change is OK with patch 3/4 together.

Will change that.

> 
> >         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];
> >         int rc;
> > -       int i;
> > -       char *str;
> > +       int i = mem_avoid_memmap_index;
> >
> > -       /* 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;
> > @@ -196,12 +159,49 @@ static void mem_avoid_memmap(void)
> >                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> >                 i++;
> >         }
> > +       mem_avoid_memmap_index = i;
> >
> >         /* More than 4 memmaps, fail kaslr */
> >         if ((i >= MAX_MEMMAP_REGIONS) && str)
> >                 memmap_too_large = true;
> >  }
> >
> > +#define COMMAND_LINE_SIZE 256
> > +static int handle_mem_memmap(void)
> > +{
> > +       char *args = (char *)get_cmd_line_ptr();
> > +       char tmp_cmdline[COMMAND_LINE_SIZE];
> 
> Can't this use a dynamic allocation instead of the 256 limit?

This is in boot/compressed code, no mm allocator built yet? Am I right?

> 
> > +       size_t len = strlen((char *)args);
> > +       char *param, *val;
> > +
> > +       len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
> > +       memcpy(tmp_cmdline, args, len);
> > +       tmp_cmdline[len] = 0;
> > +       args = tmp_cmdline;
> > +
> > +       /* Chew leading spaces */
> > +       args = skip_spaces(args);
> > +
> > +       while (*args) {
> > +               int ret;
> > +
> > +               debug_putstr(args);
> > +               debug_putstr("\n");
> 
> Are these accidentally left over?

Yes, it's for debugging. Will remove.

Thanks
Baoquan

> 
> > +
> > +               args = next_arg(args, &param, &val);
> > +               /* Stop at -- */
> > +               if (!val && strcmp(param, "--") == 0) {
> > +                       warn("Only '--' specified in cmdline");
> > +                       return -1;
> > +               }
> > +
> > +               if (!strcmp(param, "memmap"))
> > +                       mem_avoid_memmap(val);
> > +       }
> > +
> > +       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 +323,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
> >
> 
> Otherwise, yeah, this looks sensible.
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

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

[Baoquan He]

On 04/18/17 at 01:36pm, Kees Cook wrote:
> On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> > Option mem= will limit the max address system can use. Any memory
> > region above the limit will be removed. And memmap=nn[KMG] which
> > has no offset specified has the same behaviour as mem=. KASLR need
> > consider this when choose the random position for decompressing
> > kernel. Do it in this patch.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: x86@kernel.org
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > Cc: Borislav Petkov <bp@suse.de>
> > ---
> >  arch/x86/boot/compressed/kaslr.c | 53 +++++++++++++++++++++++++++++++---------
> >  1 file changed, 42 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 36ab429..5361abd 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -67,6 +67,10 @@ int mem_avoid_memmap_index;
> >  extern unsigned long get_cmd_line_ptr(void);
> >
> >
> > +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> > +unsigned long long mem_limit = ULLONG_MAX;
> 
> I would either make this 0 or ULLONG_MAX - 1 (see below).

I prefer "ULLONG_MAX-1". Please see reason below

> 
> > +
> > +
> >  enum mem_avoid_index {
> >         MEM_AVOID_ZO_RANGE = 0,
> >         MEM_AVOID_INITRD,
> > @@ -117,15 +121,18 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >                 return -EINVAL;
> >
> >         switch (*p) {
> > -       case '@':
> > -               /* Skip this region, usable */
> > -               *size = 0;
> > -               *start = 0;
> >         case '#':
> >         case '$':
> >         case '!':
> >                 *start = memparse(p + 1, &p);
> >                 return 0;
> > +       case '@':
> > +               /* Skip this region, usable */
> > +               *size = 0;
> 
> Now it looks like we're intentionally falling through. A comment
> should be included to indicate it.

Yes, as I replied in patch 2/4, it's intended. Will add comment to
indicate.

> 
> > +       default:
> > +               /* Avoid the region which is above the amount limit */
> > +               *start = 0;
> > +               return 0;
> >         }
> >
> >         return -EINVAL;
> > @@ -151,9 +158,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;
> > @@ -173,6 +185,7 @@ static int handle_mem_memmap(void)
> >         char tmp_cmdline[COMMAND_LINE_SIZE];
> >         size_t len = strlen((char *)args);
> >         char *param, *val;
> > +       u64 mem_size;
> >
> >         len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
> >         memcpy(tmp_cmdline, args, len);
> > @@ -195,8 +208,18 @@ 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)
> > +                               return -EINVAL;
> > +                       mem_limit = mem_size;
> > +               }
> >         }
> >
> >         return 0;
> > @@ -432,7 +455,8 @@ static void process_e820_entry(struct e820entry *entry,
> >  {
> >         struct mem_vector region, overlap;
> >         struct slot_area slot_area;
> > -       unsigned long start_orig;
> > +       unsigned long start_orig, end;
> > +       struct e820entry cur_entry;
> >
> >         /* Skip non-RAM entries. */
> >         if (entry->type != E820_RAM)
> > @@ -446,8 +470,15 @@ static void process_e820_entry(struct e820entry *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 - 1, mem_limit);
> > +       if (entry->addr >= end)
> > +               return;
> > +       cur_entry.addr = entry->addr;
> > +       cur_entry.size = end - entry->addr + 1;
> > +
> > +       region.start = cur_entry.addr;
> > +       region.size = cur_entry.size;
> 
> I find the manipulation of entry->addr +/- 1 confusing; it should just
> be mem_limit that is adjusted:
> 
>     end = min(entry->size + entry->addr, mem_limit + 1);

The first one is preferred. Since if no memory limit specifed, we still have
a limit, 64T-1 or UULONG_MAX -1, in logic more understandable. And in
code lines it's less.

Will change with your suggestion.

Thanks a log for your comments!

> 
> And maybe to avoid mem_limit being giant by default, maybe have "0" be special?
> 
>     cur_entry.addr = entry->addr;
>     if (mem_limit) {
>          unsigned long end = min(entry->size + entry->addr, mem_limit + 1);
>          if (entry->addr > end)
>              return;
>          cur_entry.size = end - entry->addr;
>     } else {
>           cur_entry.size = entry->size;
>     }
> 
> or something... and maybe move the whole thing earlier so other tests
> that examine entry->size are checked with the new adjusted value.
> 
> -Kees
> 
> >
> >         /* Give up if slot area array is full. */
> >         while (slot_area_index < MAX_SLOT_AREA) {
> > @@ -461,7 +492,7 @@ static void process_e820_entry(struct e820entry *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
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline

[Kees Cook]

On Tue, Apr 18, 2017 at 3:52 PM, Baoquan He <bhe@redhat.com> wrote:
> On 04/18/17 at 01:22pm, Kees Cook wrote:
>> > +#define COMMAND_LINE_SIZE 256
>> > +static int handle_mem_memmap(void)
>> > +{
>> > +       char *args = (char *)get_cmd_line_ptr();
>> > +       char tmp_cmdline[COMMAND_LINE_SIZE];
>>
>> Can't this use a dynamic allocation instead of the 256 limit?
>
> This is in boot/compressed code, no mm allocator built yet? Am I right?

misc.c uses malloc for phdrs, and the boot_heap is create to build an
area for those calls, see include/linux/decompress/mm.h. I *think* it
should be safe to use malloc here. It should be a pretty small
allocation normally.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline

[Baoquan He]

On 04/18/17 at 04:32pm, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 3:52 PM, Baoquan He <bhe@redhat.com> wrote:
> > On 04/18/17 at 01:22pm, Kees Cook wrote:
> >> > +#define COMMAND_LINE_SIZE 256
> >> > +static int handle_mem_memmap(void)
> >> > +{
> >> > +       char *args = (char *)get_cmd_line_ptr();
> >> > +       char tmp_cmdline[COMMAND_LINE_SIZE];
> >>
> >> Can't this use a dynamic allocation instead of the 256 limit?
> >
> > This is in boot/compressed code, no mm allocator built yet? Am I right?
> 
> misc.c uses malloc for phdrs, and the boot_heap is create to build an
> area for those calls, see include/linux/decompress/mm.h. I *think* it
> should be safe to use malloc here. It should be a pretty small
> allocation normally.

Yes, didn't notice this. Will use it to do dynamic malloc.
Thanks for telling!

Re: [PATCH 0/4] Handle memmap and mem kernel options in boot stage kaslr

[Baoquan He]

On 04/18/17 at 02:51pm, Ingo Molnar wrote:
> > > I ported this series to tip:x86/boot (please post future versions against that), 
> > > and beyond a trivial conflict with e820entry => e820_entry, it fails to 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
> > > 
> > > ... which could be due to bad relocations, but I've not dug any further.
> > 
> > Thanks, Ingo!
> > 
> > I will find a x86_32 system to try allmodconfig.
> 
> No need, on a 64-bit system just do:
> 
> 	make ARCH=i386 allmodconfig
> 
> and build the kernel the regular way.

Thanks, trying.

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

[Baoquan He]

On 04/18/17 at 01:36pm, Kees Cook wrote:
> On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> > @@ -432,7 +455,8 @@ static void process_e820_entry(struct e820entry *entry,
> >  {
> >         struct mem_vector region, overlap;
> >         struct slot_area slot_area;
> > -       unsigned long start_orig;
> > +       unsigned long start_orig, end;
> > +       struct e820entry cur_entry;
> >
> >         /* Skip non-RAM entries. */
> >         if (entry->type != E820_RAM)
> > @@ -446,8 +470,15 @@ static void process_e820_entry(struct e820entry *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 - 1, mem_limit);
> > +       if (entry->addr >= end)
> > +               return;
> > +       cur_entry.addr = entry->addr;
> > +       cur_entry.size = end - entry->addr + 1;
> > +
> > +       region.start = cur_entry.addr;
> > +       region.size = cur_entry.size;
> 
> I find the manipulation of entry->addr +/- 1 confusing; it should just
> be mem_limit that is adjusted:
> 
>     end = min(entry->size + entry->addr, mem_limit + 1);

Oh, it should be like that. E.g if specify mem=4096M, it means available
memory region are 0~4096M-1, or [0, 4096M). Here mem_limit = 4096M.
Adding 1 could make it wrong.

> 
> And maybe to avoid mem_limit being giant by default, maybe have "0" be special?
> 
>     cur_entry.addr = entry->addr;
>     if (mem_limit) {
>          unsigned long end = min(entry->size + entry->addr, mem_limit + 1);
>          if (entry->addr > end)
>              return;
>          cur_entry.size = end - entry->addr;
>     } else {
>           cur_entry.size = entry->size;
>     }
> 
> or something... and maybe move the whole thing earlier so other tests
> that examine entry->size are checked with the new adjusted value.

Sorry, forget replying to this comment. I am fine with moving it
earlier. In fact I put it here because there are many non-RAM e820
entries below 4G, like ACPI, for them we even don't need check limit
by the help of below check filtering. Maybe move it after below check?

        /* Skip non-RAM entries. */
        if (entry->type != E820_RAM)
                return;

> 
> -Kees
> 
> >
> >         /* Give up if slot area array is full. */
> >         while (slot_area_index < MAX_SLOT_AREA) {
> > @@ -461,7 +492,7 @@ static void process_e820_entry(struct e820entry *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
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

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

[Baoquan He]

On 04/19/17 at 08:50am, Baoquan He wrote:
> On 04/18/17 at 01:36pm, Kees Cook wrote:
> > On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He <bhe@redhat.com> wrote:
> > > @@ -432,7 +455,8 @@ static void process_e820_entry(struct e820entry *entry,
> > >  {
> > >         struct mem_vector region, overlap;
> > >         struct slot_area slot_area;
> > > -       unsigned long start_orig;
> > > +       unsigned long start_orig, end;
> > > +       struct e820entry cur_entry;
> > >
> > >         /* Skip non-RAM entries. */
> > >         if (entry->type != E820_RAM)
> > > @@ -446,8 +470,15 @@ static void process_e820_entry(struct e820entry *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 - 1, mem_limit);
> > > +       if (entry->addr >= end)
> > > +               return;
> > > +       cur_entry.addr = entry->addr;
> > > +       cur_entry.size = end - entry->addr + 1;
> > > +
> > > +       region.start = cur_entry.addr;
> > > +       region.size = cur_entry.size;
> > 
> > I find the manipulation of entry->addr +/- 1 confusing; it should just
> > be mem_limit that is adjusted:
> > 
> >     end = min(entry->size + entry->addr, mem_limit + 1);
> 
> Oh, it should be like that. E.g if specify mem=4096M, it means available
	       ^not
> memory region are 0~4096M-1, or [0, 4096M). Here mem_limit = 4096M.
> Adding 1 could make it wrong.
> 
> > 
> > And maybe to avoid mem_limit being giant by default, maybe have "0" be special?
> > 
> >     cur_entry.addr = entry->addr;
> >     if (mem_limit) {
> >          unsigned long end = min(entry->size + entry->addr, mem_limit + 1);
> >          if (entry->addr > end)
> >              return;
> >          cur_entry.size = end - entry->addr;
> >     } else {
> >           cur_entry.size = entry->size;
> >     }
> > 
> > or something... and maybe move the whole thing earlier so other tests
> > that examine entry->size are checked with the new adjusted value.
> 
> Sorry, forget replying to this comment. I am fine with moving it
> earlier. In fact I put it here because there are many non-RAM e820
> entries below 4G, like ACPI, for them we even don't need check limit
> by the help of below check filtering. Maybe move it after below check?
> 
>         /* Skip non-RAM entries. */
>         if (entry->type != E820_RAM)
>                 return;
> 
> > 
> > -Kees
> > 
> > >
> > >         /* Give up if slot area array is full. */
> > >         while (slot_area_index < MAX_SLOT_AREA) {
> > > @@ -461,7 +492,7 @@ static void process_e820_entry(struct e820entry *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
> > >
> > 
> > 
> > 
> > -- 
> > Kees Cook
> > Pixel Security

Re: [PATCH 0/4] Handle memmap and mem kernel options in boot stage kaslr

[Baoquan He]

On 04/18/17 at 02:51pm, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > On 04/18/17 at 11:47am, Ingo Molnar wrote:
> > > 
> > > * Baoquan He <bhe@redhat.com> wrote:
> > > 
> > > > 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.
> > > > 
> > > > Baoquan He (4):
> > > >   param: Move function next_arg to lib/cmdline.c for later reuse
> > > >   KASLR: Parse all memmap entries in cmdline
> > > >   KASLR: Handle memory limit specified by memmap and mem option
> > > >   doc: Update description about memmap option in kernel-parameter.txt
> > > > 
> > > >  Documentation/admin-guide/kernel-parameters.txt |   9 ++
> > > >  arch/x86/boot/compressed/cmdline.c              |   2 +-
> > > >  arch/x86/boot/compressed/kaslr.c                | 161 ++++++++++++++----------
> > > >  arch/x86/boot/string.c                          |   8 ++
> > > >  include/linux/kernel.h                          |   1 +
> > > >  kernel/params.c                                 |  52 --------
> > > >  lib/cmdline.c                                   |  57 +++++++++
> > > >  7 files changed, 172 insertions(+), 118 deletions(-)
> > > 
> > > I ported this series to tip:x86/boot (please post future versions against that), 
> > > and beyond a trivial conflict with e820entry => e820_entry, it fails to 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
> > > 
> > > ... which could be due to bad relocations, but I've not dug any further.
> > 
> > Thanks, Ingo!
> > 
> > I will find a x86_32 system to try allmodconfig.
> 
> No need, on a 64-bit system just do:
> 
> 	make ARCH=i386 allmodconfig
> 
> and build the kernel the regular way.

Sorry for late update. I tried on a x86 64 system with "make ARCH=i386
allmodconfig", and saw the ld warning.  I added a '-r' after -pie, the
building passed. Seems including lib/ctype.c or lib/cmdline.c caused
this. Will dig further to see what's going on.

ifeq ($(CONFIG_X86_32),y)      
LDFLAGS += $(call ld-option, -pie -r) $(call ld-option, --no-dynamic-linker)
else

Thanks
Baoquan

Re: [PATCH 0/4] Handle memmap and mem kernel options in boot stage kaslr

[Baoquan He]

On 04/18/17 at 02:51pm, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > On 04/18/17 at 11:47am, Ingo Molnar wrote:
> > > I ported this series to tip:x86/boot (please post future versions against that), 
> > > and beyond a trivial conflict with e820entry => e820_entry, it fails to 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
> > > 
> > > ... which could be due to bad relocations, but I've not dug any further.
> > 
> > Thanks, Ingo!
> > 
> > I will find a x86_32 system to try allmodconfig.
> 
> No need, on a 64-bit system just do:
> 
> 	make ARCH=i386 allmodconfig

Those EXPORT_SYMBOL(sym) in lib/ctype.c|cmdline.c caused this build
failure. I added below code to skip linux/export.h including, it works.
However I haven't found out why EXPORT_SYMBOL in boot stage caused the
failure.

#define _LINUX_EXPORT_H
#define EXPORT_SYMBOL(sym)

> 
> and build the kernel the regular way.
> 
> Thanks,
> 
> 	Ingo